separates durable nonce and blockhash domains #25744

behzadnouri · 2022-06-02T21:07:06Z

Problem

AdvanceNonceAccount instruction updates nonce to blockhash. This makes it
possible that a durable transaction is executed twice both as a normal
transaction and a nonce transaction if it uses a recent blockhash (as opposed to
the durable nonce) for its recent_blockhash field.

Summary of Changes

The commit prevents this double execution by separating nonce and blockhash
domains; when advancing nonce account, blockhash is hashed with a fixed string.
As a result a blockhash cannot be a valid nonce value; and if transaction was
once executed as a normal transaction it cannot be re-executed as a durable
transaction again and vice-versa.

To add type-safety and prevent old blockhash Hash values creeping into nonce
accounts the commit adds a new DurableNonce type replacing previous
blockhash value in nonce accounts state.

The code to separate durable nonce and blockhash domains is feature gated.
A 2nd feature will enable durable nonce at least one epoch after 1st feature is
activated.
By the time 2nd feature is activated, some nonce accounts will have an
old blockhash, but no nonce account can have a recent blockhash.
As a result no transaction (durable or normal) can be executed twice.

closes #25711
Feature Gate Issue: #25772 #25773

t-nelson · 2022-06-02T21:12:14Z

we're going to need to hash in initialize and withdraw as well

behzadnouri · 2022-06-02T21:41:23Z

we're going to need to hash in initialize and withdraw as well

right, done.

jstarry · 2022-06-02T21:42:38Z

runtime/src/bank.rs

+        let enable_durable_nonce = self
+            .feature_set
+            .is_active(&feature_set::separate_nonce_from_blockhash::id());


I think we need two feature activations still because until separate_nonce_from_blockhash is activated, nonce accounts will still be storing raw blockhashes. As long as a recent blockhash could be stored in a nonce account, we cannot re-enable durable nonces.

But after this feature activation, if a transaction is executed once as either normal or durable transaction, then its nonce will not be equal to a blockhash. So it cannot be re-exectued, no?

It can still be re-executed because the durable tx would be processed as a normal tx and if it failed, the advance nonce ix state changes would be reverted.

~~could continue storing the raw blockhash, but switch domains on compare~~

n/m, that brings back the client-side breakage

How would a 2nd feature activation help though?

If we use two feature activations, we could wait an additional epoch before nonces are re-enabled to make sure it would be impossible for a nonce account to have a recent blockhash stored right before the separate_nonce_from_blockhash feature is activated.

I'm sure we can solve this without a second feature though. For instance, I don't think we use the fee calculator value in the nonce data anymore and we could use that data to flag nonce hashes that use the new scheme and only allow processing those. If a nonce didn't have that flag, it would have to be advanced before it could be re-used. This be a headache though for exchanges and users though.

I don't think we use the fee calculator value in the nonce data anymore and we could use that data to flag nonce hashes that use the new scheme and only allow processing those

Never mind, looks like it's still used. How about using a new version for the nonce state so that we can enforce that only the new type of nonce hashes can be accepted? Right now we have no way to distinguish.

Since you've added the 2nd feature, we shouldn't need to worry about versioning nonce states.

runtime/src/nonce_keyed_account.rs

jstarry · 2022-06-02T21:47:08Z

runtime/src/bank.rs

+        let enable_durable_nonce = self
+            .feature_set
+            .is_active(&feature_set::separate_nonce_from_blockhash::id());


It can still be re-executed because the durable tx would be processed as a normal tx and if it failed, the advance nonce ix state changes would be reverted.

t-nelson · 2022-06-02T22:05:27Z

Also the rollback hash 😅

Welcome to Durable Nonce Hell!

jstarry · 2022-06-02T22:11:48Z

Also the rollback hash 😅

Welcome to Durable Nonce Hell!

Great catch, this is why I propose that we use a wrapper struct struct Nonce(Hash) so that any direct use of a blockhash will cause a type error instead of being silently accepted. Even if we catch all the uses now, without type safety it will be too easy to mess this up in future changes as well.

t-nelson · 2022-06-02T22:31:05Z

agreed that type safety is desired here. not worth blocking on though IMO. follow up is fine

jstarry · 2022-06-02T22:42:11Z

agreed that type safety is desired here. not worth blocking on though IMO. follow up is fine

Could be worth blocking on if it reveals another edge case we forgot about

behzadnouri · 2022-06-02T22:43:05Z

agreed that type safety is desired here. not worth blocking on though IMO. follow up is fine

Could be worth blocking on if it reveals another edge case we forgot about

yes, I am adding a new type.

behzadnouri · 2022-06-02T23:31:12Z

Also the rollback hash sweat_smile

Welcome to Durable Nonce Hell!

done

jstarry

The type safety changes look great, just concerned about the feature activation re-introducing the bug for a short amount of time.

AdvanceNonceAccount instruction updates nonce to blockhash. This makes it possible that a durable transaction is executed twice both as a normal transaction and a nonce transaction if it uses blockhash (as opposed to nonce) for its recent_blockhash field. The commit prevents this double execution by separating nonce and blockhash domains; when advancing nonce account, blockhash is hashed with a fixed string. As a result a blockhash cannot be a valid nonce value; and if transaction was once executed as a normal transaction it cannot be re-executed as a durable transaction again and vice-versa.

Previous commit separates durable nonce and blockhash domains with a feature gate. A 2nd feature added in this commit enables durable nonce at least one epoch after the 1st feature. By the time 2nd feature is activated, some nonce accounts will have an old blockhash, but no nonce account can have a recent blockhash. As a result no transaction (durable or normal) can be executed twice.

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 blockhash domain, and permanently disables durable transactions for legacy nonces which are in blockhash domain.

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.

#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.

#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

…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 <behzadnouri@gmail.com>

…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 <behzadnouri@gmail.com>

…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.

behzadnouri added the feature-gate Pull Request adds or modifies a runtime feature gate label Jun 2, 2022

behzadnouri force-pushed the advance-nonce branch 2 times, most recently from f505972 to dcdb940 Compare June 2, 2022 21:10

behzadnouri force-pushed the advance-nonce branch from dcdb940 to 4e6ec83 Compare June 2, 2022 21:40

jstarry reviewed Jun 2, 2022

View reviewed changes

jstarry mentioned this pull request Jun 2, 2022

flips nonce/non-nonce blockhash check #25711

Closed

jstarry reviewed Jun 3, 2022

View reviewed changes

behzadnouri changed the title ~~separates nonce and blockhash domains~~ separates durable nonce and blockhash domains Jun 3, 2022

behzadnouri force-pushed the advance-nonce branch 4 times, most recently from cac1120 to f7a5c8e Compare June 3, 2022 19:27

This was referenced Jun 3, 2022

Feature Gate: separate durable nonce and blockhash domains #25772

Closed

Feature Gate: enable durable nonce #25773

Closed

behzadnouri added 2 commits June 3, 2022 16:43

behzadnouri force-pushed the advance-nonce branch from f7a5c8e to 0463d4d Compare June 3, 2022 20:43

behzadnouri added v1.9 labels Jun 3, 2022

behzadnouri requested review from t-nelson and jstarry June 3, 2022 22:49

jstarry mentioned this pull request Jun 8, 2022

Nonce account migration and re-enabling durable nonce transactions #25787

Closed

30 tasks

aeyakovenko mentioned this pull request Jun 9, 2022

Nonce fix progress #25886

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separates durable nonce and blockhash domains #25744

separates durable nonce and blockhash domains #25744

behzadnouri commented Jun 2, 2022 •

edited

Loading

t-nelson commented Jun 2, 2022

behzadnouri commented Jun 2, 2022

jstarry Jun 2, 2022

behzadnouri Jun 2, 2022 •

edited

Loading

jstarry Jun 2, 2022

t-nelson Jun 2, 2022 •

edited

Loading

behzadnouri Jun 2, 2022

jstarry Jun 3, 2022

jstarry Jun 3, 2022

jstarry Jun 3, 2022

jstarry Jun 2, 2022

t-nelson commented Jun 2, 2022

jstarry commented Jun 2, 2022

t-nelson commented Jun 2, 2022

jstarry commented Jun 2, 2022

behzadnouri commented Jun 2, 2022

behzadnouri commented Jun 2, 2022

jstarry left a comment

separates durable nonce and blockhash domains #25744

separates durable nonce and blockhash domains #25744

Conversation

behzadnouri commented Jun 2, 2022 • edited Loading

Problem

Summary of Changes

t-nelson commented Jun 2, 2022

behzadnouri commented Jun 2, 2022

jstarry Jun 2, 2022

Choose a reason for hiding this comment

behzadnouri Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

jstarry Jun 2, 2022

Choose a reason for hiding this comment

t-nelson Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

behzadnouri Jun 2, 2022

Choose a reason for hiding this comment

jstarry Jun 3, 2022

Choose a reason for hiding this comment

jstarry Jun 3, 2022

Choose a reason for hiding this comment

jstarry Jun 3, 2022

Choose a reason for hiding this comment

jstarry Jun 2, 2022

Choose a reason for hiding this comment

t-nelson commented Jun 2, 2022

jstarry commented Jun 2, 2022

t-nelson commented Jun 2, 2022

jstarry commented Jun 2, 2022

behzadnouri commented Jun 2, 2022

behzadnouri commented Jun 2, 2022

jstarry left a comment

Choose a reason for hiding this comment

behzadnouri commented Jun 2, 2022 •

edited

Loading

behzadnouri Jun 2, 2022 •

edited

Loading

t-nelson Jun 2, 2022 •

edited

Loading