permanently disables durable nonces with chain blockhash domain #25788

behzadnouri · 2022-06-05T19:38:35Z

Problem

#25744
separated durable nonce and blockhash domains, which will stop double
execution going forward. However it is possible that a durable
transaction has already been executed once as a normal transaction and
it is now a valid durable transaction. #25744 cannot stop such
transactions to be re-executed until the nonce accounts are advanced.

Summary of Changes

This commit adds a new nonce version indicating that the nonce is moved
out of blockhash domain, and permanently disables durable transactions
for legacy nonces which are in blockhash domain.

solana-labs#25788 permanently disables durable transactions with legacy nonce versions wich are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

solana-labs#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

sdk/program/src/nonce/state/mod.rs

solana-labs#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

behzadnouri · 2022-06-06T13:11:07Z

send-transaction-service/src/send_transaction_service.rs

-                if !nonce_account::verify_nonce_account(&nonce_account, &durable_nonce)
-                    && signature_status.is_none()
-                    && expired
-                {


This condition is weird; shouldn't these be || instead of &&?
cc @CriesofCarrots

The intent is to retry sending nonce transactions until the nonce is advanced or the last_valid_block_height passed in from rpc is reached.
If the nonce has been advanced, but signature_status.is_some(), the service keeps the transaction to check status until it's rooted (handled in signature_status match below), so that's why the initial &&
The expired clause was added by @lijunwangs ; he might be able to explain it faster.

This logic ensures the nonce transaction to have allocated time to finish before being dropped altogether. Without this check, the transaction might be dropped as expired whenever the retry loop is run even right after the transaction was sent in the send thread.

Mm, to confirm, you were seeing situations where the nonce had been advanced from a successful transaction but the signature_status wasn't available yet?

I am still not sure why we keep onto the transaction even if verify_nonce_account returns false?

verify_nonce_account returns false when the nonce has been advanced, which happens on both successful and failed durable-nonce transactions.
I think @lijunwangs is saying he saw instances where a successful tx would return None for the check on L592 because the signature wasn't yet available, but where the nonce had already been advanced by the time verify_nonce_account ran on L600, which means the tx should have been preserved to be checked until root. But best for him to confirm that I'm understanding correctly.

If the transaction did not make it to the local node yet, line 594 will return a default account, and verify_nonce_account will return false if I understand correctly. This extra "expired" check ensure we at least wait for one cycle before we determine it is expired. This was introduced to reduce any regression potential when we did refactoring in https://github.com/solana-labs/solana/pull/24083/files#diff-a3caa5df6489d5460449094b87e57398a081eb6d7feca10bf89fa5847b429251R396.

Hmm, you're trying to preserve transactions attempting to use a nonce account that hasn't been created yet? Doesn't seem quite right. But there's really very little potential harm in the && expired check

behzadnouri · 2022-06-06T13:39:48Z

runtime/src/nonce_keyed_account.rs

+            account.set_state(&Versions::new(
+                State::Initialized(new_data),
+                separate_domains,
+            ))


AuthorizeNonceAccount does not change the value of nonce.
So this updated code is preserving domain and not bumping nonce out of blockhash domain because otherwise it needs to change the value of durable_nonce and that is a change of behavior.

Can we change the behavior here and change the nonce as well? cc @t-nelson

I personally think we should preserve the version and the blockhash

solana-labs#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

…ions solana-labs#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

solana-labs#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

#25744 separated durable nonce and blockhash domains, which will stop double execution going forward. However it is possible that a durable transaction has *already* been executed once as a normal transaction and it is now a valid durable transaction. #25744 cannot stop such transactions to be re-executed until the nonce accounts are advanced. This commit adds a new nonce version indicating that the nonce is moved out of the blockhash domain, and permanently disables durable transactions for legacy nonces which are in the blockhash domain. (cherry picked from commit 3c1ce3c) # Conflicts: # cli/src/nonce.rs # rpc/src/rpc.rs # runtime/src/nonce_keyed_account.rs # runtime/src/system_instruction_processor.rs # sdk/program/src/nonce/state/current.rs # send-transaction-service/src/send_transaction_service.rs

#25744 separated durable nonce and blockhash domains, which will stop double execution going forward. However it is possible that a durable transaction has *already* been executed once as a normal transaction and it is now a valid durable transaction. #25744 cannot stop such transactions to be re-executed until the nonce accounts are advanced. This commit adds a new nonce version indicating that the nonce is moved out of the blockhash domain, and permanently disables durable transactions for legacy nonces which are in the blockhash domain. (cherry picked from commit 3c1ce3c) # Conflicts: # runtime/src/nonce_keyed_account.rs # sdk/program/src/nonce/state/current.rs

solana-labs#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

…port #25788) (#25872) * permanently disables durable nonces with chain blockhash domain (#25788) #25744 separated durable nonce and blockhash domains, which will stop double execution going forward. However it is possible that a durable transaction has *already* been executed once as a normal transaction and it is now a valid durable transaction. #25744 cannot stop such transactions to be re-executed until the nonce accounts are advanced. This commit adds a new nonce version indicating that the nonce is moved out of the blockhash domain, and permanently disables durable transactions for legacy nonces which are in the blockhash domain. (cherry picked from commit 3c1ce3c) # Conflicts: # runtime/src/nonce_keyed_account.rs # sdk/program/src/nonce/state/current.rs * removes mergify merge conflicts Co-authored-by: behzad nouri <[email protected]>

…port #25788) (#25871) * permanently disables durable nonces with chain blockhash domain (#25788) #25744 separated durable nonce and blockhash domains, which will stop double execution going forward. However it is possible that a durable transaction has *already* been executed once as a normal transaction and it is now a valid durable transaction. #25744 cannot stop such transactions to be re-executed until the nonce accounts are advanced. This commit adds a new nonce version indicating that the nonce is moved out of the blockhash domain, and permanently disables durable transactions for legacy nonces which are in the blockhash domain. (cherry picked from commit 3c1ce3c) # Conflicts: # cli/src/nonce.rs # rpc/src/rpc.rs # runtime/src/nonce_keyed_account.rs # runtime/src/system_instruction_processor.rs # sdk/program/src/nonce/state/current.rs # send-transaction-service/src/send_transaction_service.rs * removes mergify merge conflicts Co-authored-by: behzad nouri <[email protected]>

solana-labs#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

…#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

…#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain. (cherry picked from commit b419031) # Conflicts: # runtime/src/system_instruction_processor.rs # sdk/program/src/system_instruction.rs # web3.js/src/system-program.ts

…#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain. (cherry picked from commit b419031) # Conflicts: # sdk/program/src/system_instruction.rs # web3.js/src/system-program.ts

… (#25789) solana-labs/solana#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

…5789) (#25890) * feat(nonce): adds system instruction to upgrade legacy nonce versions (#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain. (cherry picked from commit b419031) # Conflicts: # runtime/src/system_instruction_processor.rs # sdk/program/src/system_instruction.rs # web3.js/src/system-program.ts * removes mergify merge conflicts * backport UpgradeNonceAccount Co-authored-by: behzad nouri <[email protected]>

…5789) (#25891) * feat(nonce): adds system instruction to upgrade legacy nonce versions (#25789) #25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain. (cherry picked from commit b419031) # Conflicts: # sdk/program/src/system_instruction.rs # web3.js/src/system-program.ts * removes mergify merge conflicts * backport UpgradeNonceAccount Co-authored-by: behzad nouri <[email protected]>

…na-labs#25788) solana-labs#25744 separated durable nonce and blockhash domains, which will stop double execution going forward. However it is possible that a durable transaction has *already* been executed once as a normal transaction and it is now a valid durable transaction. solana-labs#25744 cannot stop such transactions to be re-executed until the nonce accounts are advanced. This commit adds a new nonce version indicating that the nonce is moved out of the blockhash domain, and permanently disables durable transactions for legacy nonces which are in the blockhash domain.

… (#25789) solana-labs/solana#25788 permanently disables durable transactions with legacy nonce versions which are within chain blockhash domain. This commit adds a new system instruction for a one-time idempotent upgrade of legacy nonce accounts in order to bump them out of chain blockhash domain.

behzadnouri force-pushed the disable-legacy-nonce branch 3 times, most recently from e02e4b8 to 2aa987c Compare June 5, 2022 21:12

behzadnouri mentioned this pull request Jun 5, 2022

adds system instruction to upgrade legacy nonce versions #25789

Merged

jstarry reviewed Jun 5, 2022

View reviewed changes

sdk/program/src/nonce/state/mod.rs Outdated Show resolved Hide resolved

behzadnouri force-pushed the disable-legacy-nonce branch from 2aa987c to 799a60e Compare June 5, 2022 23:50

behzadnouri requested review from CriesofCarrots, jstarry, mvines, ryoqun, sakridge and t-nelson June 6, 2022 02:10

behzadnouri added v1.9 labels Jun 6, 2022

behzadnouri force-pushed the disable-legacy-nonce branch from 799a60e to 8a6eec3 Compare June 6, 2022 13:18

behzadnouri commented Jun 6, 2022

View reviewed changes

behzadnouri force-pushed the disable-legacy-nonce branch from 8a6eec3 to 682aa12 Compare June 6, 2022 13:20

behzadnouri changed the title ~~permanently disables durable transactions with chain blockhash domain~~ permanently disables durable nonces with chain blockhash domain Jun 6, 2022

behzadnouri force-pushed the disable-legacy-nonce branch from 682aa12 to 2254f96 Compare June 6, 2022 13:36

behzadnouri commented Jun 6, 2022

View reviewed changes

behzadnouri force-pushed the disable-legacy-nonce branch 2 times, most recently from b9196e0 to a4a7084 Compare June 6, 2022 14:08

mergify bot mentioned this pull request Jun 9, 2022

permanently disables durable nonces with chain blockhash domain (backport #25788) #25871

Merged

mergify bot mentioned this pull request Jun 9, 2022

permanently disables durable nonces with chain blockhash domain (backport #25788) #25872

Merged

aeyakovenko mentioned this pull request Jun 9, 2022

Nonce fix progress #25886

Closed

11 tasks

jackcmay mentioned this pull request Jun 13, 2022

Confusing durable nonce function call convention #25950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

permanently disables durable nonces with chain blockhash domain #25788

permanently disables durable nonces with chain blockhash domain #25788

behzadnouri commented Jun 5, 2022

behzadnouri Jun 6, 2022

CriesofCarrots Jun 6, 2022

lijunwangs Jun 6, 2022 •

edited

Loading

CriesofCarrots Jun 6, 2022

behzadnouri Jun 6, 2022

CriesofCarrots Jun 7, 2022

lijunwangs Jun 7, 2022

CriesofCarrots Jun 7, 2022

behzadnouri Jun 6, 2022

jstarry Jun 6, 2022

permanently disables durable nonces with chain blockhash domain #25788

permanently disables durable nonces with chain blockhash domain #25788

Conversation

behzadnouri commented Jun 5, 2022

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lijunwangs Jun 6, 2022 • edited Loading

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

lijunwangs Jun 6, 2022 •

edited

Loading