Add `deprecate_deferred_transactions` protocol feature #8697

johndebord · 2020-02-24T22:57:53Z

1st Protocol Feature `stop_deferred_transactions`
=================================================

[X]  = completed
[XX] = double-checked
[?]  = seems to be covered if I've implemented the above spec correctly

[XX] `cancel_deferred`
[XX]   - shall continue working
[XX] `send_deferred`
[XX]   - If `replace_existing` == true in `send_deferred`; it shall be a noop
[XX] `canceldelay` native action shall not change
[XX] `schedule_deferred_transaction`
[XX]   - If there is no transaction to replace:
           fail
[XX]   - If there is a transaction to replace:
           continue to cancel the old one but do not schedule the new one

[XX] `delaysec` > 0 is not allowed
[XX]   - Contract-generated deferred shall not assert in native code (`send_deferred`)
[XX]   - User-generated deferred shall assert in native code (`controller.cpp:push_transaction` (not `schedule_transaciton`))
[?]    - As soon as the protocol feature is activated there shall be no more ways to
           insert deferred transactions into the generated transaction table(s)
[?]    - Only if a deferred transaction is expired can it be pushed (`controller.cpp:push_schedule_transaction`)

Testing:
[XX] The fixture is going to have to be changed (don't activate all protocol features right away)
[XX] A few tests test to see if the queue gets larger (New test shall be added to verify that it doesn't do anything)
[?]  Check that the transaction expires appropriately
[?]  Will have to have duplicate tests, testing the different combinations of turning on/off the protocol features relating to deferred transactions
[XX] 1) User-side
[XX]    - Check that a user-defined transaction cannot have a `delay_sec` greater than 0
[XX] 2) Contract-side
[XX]    - Make sure that the corresponding logic for deferred transactions works correctly

…ix `currency_tests`

swatanabe-b1

Haven't reviewed the tests yet.

swatanabe-b1 · 2020-03-25T18:48:06Z

libraries/chain/apply_context.cpp

@@ -512,29 +519,33 @@ void apply_context::schedule_deferred_transaction( const uint128_t& sender_id, a

      // Use remove and create rather than modify because mutating the trx_id field in a modifier is unsafe.
      db.remove( *ptr );


This is not a no-op, which doesn't seem consistent with the fact that send_deferred skips schedule_deferred_transaction entirely. Actually, I don't understand the motivation for allowing replace_existing.

My assumption, based off of the specification, is that there are two ways to enter deferred transaction logic:

Through transaction_api::send_deferred.

Through a user extending his/her code via a plugin and calling apply_context::schedule_deferred_transaction.

Therefore two individual (seemingly redundant) checks must be made. Am I wrong in this assumption?

I don't think apply_context::schedule_deferred_transaction should ever be called outside of wasm. Also, the main point that I was trying to make was that the behavior is not the same in the two cases.

I believe they now have the same behavior.

The behavior is still different in the case of replace_existing = true. transaction_api::send_deferred short-circuits this case to a no-op, but apply_context::schedule_deferred_transaction handles it in a more complex way.

Yes, that is what I was told to implement.

[XX] send_deferred
[XX] - If replace_existing == true in send_deferred; it shall be a noop

[XX] schedule_deferred_transaction
[XX] - If there is no transaction to replace: fail
[XX] - If there is a transaction to replace: continue to cancel the old one but do not schedule the new one

Hence, the assertion in apply_context::schedule_deferred_transaction.

Can you explain the motivation for this difference?

The motivation is unbeknownst to me. I just took it at face-value.

I'm somewhat concerned that this may be a misunderstanding. The spec for schedule_deferred_transaction sounds to me like reasonable behavior for both of them. @arhag

libraries/chain/apply_context.cpp

libraries/chain/wasm_interface.cpp

libraries/chain/protocol_feature_manager.cpp

libraries/testing/include/eosio/testing/tester.hpp

libraries/testing/tester.cpp

task_spec

swatanabe-b1 · 2020-04-01T14:46:55Z

libraries/chain/controller.cpp

+
+      bool stop_deferred_transactions_activated = self.is_builtin_activated(builtin_protocol_feature_t::stop_deferred_transactions);
+
+      if( gtrx.expiration < self.pending_block_time() || stop_deferred_transactions_activated ) {


This doesn't seem to match the spec. "Only if a deferred transaction is expired can it be pushed" You're instead treating all transactions as if they were expired.

The behavior now matches the spec; if the transaction has expired then the transaction shall expire. Then we check to see if builtin_protocol_feature_t::stop_deferred_transactions has been activated; if so, then the trace shall be returned, and no further operations are done on the transaction.

Does producer_plugin know how to handle this outcome?

libraries/testing/include/eosio/testing/tester.hpp

swatanabe-b1 · 2020-04-01T15:44:44Z

libraries/testing/include/eosio/testing/tester.hpp

@@ -379,7 +378,7 @@ namespace eosio { namespace testing {

         void schedule_protocol_features_wo_preactivation(const vector<digest_type> feature_digests);
         void preactivate_protocol_features(const vector<digest_type> feature_digests);
-         void preactivate_all_builtin_protocol_features();
+         void preactivate_builtin_protocol_features(const std::vector<builtin_protocol_feature_t>& ignored_features);


The name will be confusing at the call site. If I see:

t.preactivate_builtin_protocol_features({wtmsig_block_signatures, replace_deferred});

I would assume that it meant the features to activate, not the features to exclude. I wouldn't even bother looking for the declaration because the meaning is "obvious."

Renamed to preactivate_selected_protocol_features.

I don't see how the new name expresses the meaning of the function better than the old name. I think part of the problem is that the function's behavior is odd, which makes it hard to come up with a good name.

swatanabe-b1 · 2020-04-01T15:59:25Z

libraries/testing/tester.cpp

+            const auto dependency_is_ignored     = std::find( ignored_digests.cbegin(), ignored_digests.cend(), feature );
+            const auto dependency_has_dependency = std::find( pf.dependencies.cbegin(),  pf.dependencies.cend(),  feature );
+            if ( dependency_is_ignored != ignored_digests.cend() && dependency_has_dependency != pf.dependencies.cend() ) {
+                check_dependencies(*dependency_has_dependency);


Doesn't this causes infinite recursion? *dependency_has_dependency should be equal to feature, right? I can't quite figure out what you're trying to do.

Should now be checking protocol feature dependencies correctly.

Only handles the simple case of A depends on B, ignore B. Does not handle the possibility of chaining ignores: A -> B -> C -> D, ignore D, which needs to cause all of A, B, C, and D to be ignored.

Also I'm on the fence about whether it's better to ignore additional features or whether it's better to issue an error. On the one hand, it's bad if adding a new protocol feature breaks existing tests. On the other hand, silently disabling more protocol features than were given may be surprising. What do you think?

I definitely considered that case. But for the time being we only have the case of A depends on B.
Would it be best practice to account for future cases, such as chaining ignores? Or is the best practice only to account for what is currently in use?

And I definitely agree with you there on the last point. If it were up to me, I think tester is due for a re-write; to consider, from the ground up, an architecture where an arbitrary number of protocol features that may or may not depend on each other.

Best practice is to handle the public API of protocol features. Since arbitrary protocol feature dependencies are allowed, you should support them. Tester should not need any specific knowledge of what protocol features are actually defined. The existing code in tester to determine the activation order of protocol features is designed to handle arbitrary dependencies correctly, so I think it's reasonable for you to do so as well.

Now, with all of that having been said, being fully general is not always worth the effort. If you do decide to take shortcuts, you should definitely call it out in a comment, to help anyone who adds a new protocol feature and gets confused by mysterious failures.

libraries/testing/tester.cpp

swatanabe-b1 · 2020-04-01T16:13:12Z

unittests/api_tests.cpp

-	set_code( N(testapi), contracts::test_api_wasm() );
-	produce_blocks(1);
+BOOST_AUTO_TEST_CASE(action_receipt) { try {
+   validating_tester chain( {}, {::eosio::chain::builtin_protocol_feature_t::stop_deferred_transactions} );


The original code uses TESTER. Can you make tester define the same constructors as validating_tester?

Made matching constructors for class tester.

unittests/api_tests.cpp

unittests/currency_tests.cpp

swatanabe-b1 · 2020-04-06T14:31:04Z

libraries/chain/apply_context.cpp

@@ -522,8 +528,22 @@ void apply_context::schedule_deferred_transaction( const uint128_t& sender_id, a
         gtx.expiration  = gtx.delay_until + fc::seconds(control.get_global_properties().configuration.deferred_trx_expiration_window);

         trx_size = gtx.set( trx );
+


So this now allows deferred transactions to be replaced. I'm not so sure that that's a good idea, since it allows the expiration to be arbitrarily extended as long as someone keeps replacing it.

libraries/chain/apply_context.cpp

johndebord and others added 24 commits February 24, 2020 17:55

Add deprecate_deferred_transactions protocol feature

03ad47d

Push changes; add exception

6493043

Pushing changes

5a4c580

Pushing changes

738859e

Pushing changes for the day

318c668

Update diff to locate bug

f5469bb

Update diff to locate bug

e0a19c3

Update diff to locate bug

fcf6d85

Update diff to locate bug

3a56333

Update validating_tester

d05174b

Add notes

2c2fdc0

Pushing changes for debugging

d80ae1f

delay_tests passing

9ecb930

protocol_feature_tests passing

4a855aa

Trying a different strategy for testing proto feature activation

cd6e39f

Checkpoint0

7c57d60

Checkpoint1

efd7640

Checkpoint2: All tests passing

45c073d

Checkpoint3

a99c7c1

Testing git repo watcher script0

5fa34b8

Cleanup

bbaf657

protocol_feature_tests passing again

963cfbb

All tests passing

01a34aa

Ready for review

6c9f0c1

johndebord force-pushed the deprecate-deferred-protocol-feature branch 2 times, most recently from ccf3f18 to 6c9f0c1 Compare March 24, 2020 18:31

johndebord added 2 commits March 24, 2020 18:01

Merge develop

c9e0439

Fix currency_tests

1a4af45

nksanthosh requested a review from swatanabe-b1 March 25, 2020 18:01

Move validating_tester constructors to more readable location and f…

f6a3d85

…ix `currency_tests`

swatanabe-b1 suggested changes Mar 25, 2020

View reviewed changes

johndebord added 5 commits March 25, 2020 18:06

eos-vm-oc tests fix attempt0

6fe80c4

Post code review0

9cb5a21

Post code review1

b7d4e2c

Post code review2

310154a

Post code review2

91ec090

swatanabe-b1 reviewed Apr 1, 2020

View reviewed changes

johndebord added 2 commits April 1, 2020 20:58

|

36bdc74

|

d619846

swatanabe-b1 reviewed Apr 6, 2020

View reviewed changes

johndebord added 12 commits April 6, 2020 13:03

Merge develop

460d80b

|

084e7bf

|

ba86dd6

|

e671033

Making sure both protocol features are checked consistently

a89b9b7

Tests working with both protocol features

fe76dc6

Merge develop

418f91b

Push weekend changes

ee0efc0

Pushing changes; generated transaction object table deletion

1ef5c18

|

203e725

Failing on light_validation_skip_cfa; attempting to fix

531f005

Fix light_validation_skip_cfa test

ae25ee7

swatanabe-b1 mentioned this pull request Apr 24, 2020

Make wasm constraints configurable #8360

Merged

3 tasks

heifner mentioned this pull request May 10, 2022

Remove Deferred/Scheduled transactions eosnetworkfoundation/mandel#206

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `deprecate_deferred_transactions` protocol feature #8697

Add `deprecate_deferred_transactions` protocol feature #8697

johndebord commented Feb 24, 2020 •

edited

Loading

swatanabe-b1 left a comment

swatanabe-b1 Mar 25, 2020

johndebord Mar 27, 2020

swatanabe-b1 Mar 27, 2020

johndebord Apr 6, 2020

swatanabe-b1 Apr 6, 2020

johndebord Apr 6, 2020

swatanabe-b1 Apr 6, 2020

johndebord Apr 6, 2020

swatanabe-b1 Apr 6, 2020

swatanabe-b1 Apr 1, 2020

johndebord Apr 6, 2020

swatanabe-b1 Apr 6, 2020

swatanabe-b1 Apr 1, 2020

johndebord Apr 6, 2020

swatanabe-b1 Apr 6, 2020

swatanabe-b1 Apr 1, 2020

johndebord Apr 6, 2020

swatanabe-b1 Apr 6, 2020

johndebord Apr 6, 2020

swatanabe-b1 Apr 6, 2020

swatanabe-b1 Apr 1, 2020

johndebord Apr 6, 2020

swatanabe-b1 Apr 6, 2020

		@@ -512,29 +519,33 @@ void apply_context::schedule_deferred_transaction( const uint128_t& sender_id, a

		// Use remove and create rather than modify because mutating the trx_id field in a modifier is unsafe.
		db.remove( *ptr );


		bool stop_deferred_transactions_activated = self.is_builtin_activated(builtin_protocol_feature_t::stop_deferred_transactions);

		if( gtrx.expiration < self.pending_block_time() \|\| stop_deferred_transactions_activated ) {

		@@ -522,8 +528,22 @@ void apply_context::schedule_deferred_transaction( const uint128_t& sender_id, a
		gtx.expiration = gtx.delay_until + fc::seconds(control.get_global_properties().configuration.deferred_trx_expiration_window);

		trx_size = gtx.set( trx );

Add deprecate_deferred_transactions protocol feature #8697

Are you sure you want to change the base?

Add deprecate_deferred_transactions protocol feature #8697

Conversation

johndebord commented Feb 24, 2020 • edited Loading

swatanabe-b1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `deprecate_deferred_transactions` protocol feature #8697

Add `deprecate_deferred_transactions` protocol feature #8697

johndebord commented Feb 24, 2020 •

edited

Loading