flips nonce/non-nonce blockhash check #25711

behzadnouri · 2022-06-01T19:31:16Z

Problem

Durable nonce transactions can be executed twice.

Summary of Changes

gist is to flip the nonce/non-nonce blockhash check so that whether a
durable nonce transaction is flagged is not dependent up whether the
blockhash is extant in the rbh queue

jstarry

Lgtm

Pull request has been modified.

CriesofCarrots

Lgtm, thanks, Behzad!

Pull request has been modified.

t-nelson

lgtm. thanks!

were you able to get the test fixed up?

t-nelson · 2022-06-01T22:35:43Z

runtime/src/bank.rs

+            || self.cluster_type() != ClusterType::MainnetBeta
+            || self.slot() <= 135986379)
+            .then(|| self.check_message_for_nonce(tx.message()))
+            .flatten()


ah, flatten! that's what i was looking for 🙂

behzadnouri · 2022-06-01T22:51:27Z

were you able to get the test fixed up?

no, can't produce failure on master.

t-nelson · 2022-06-02T03:05:05Z

no, can't produce failure on master.

i'll try to fix up the test this evening

t-nelson

sry, changing my mind. this change is insufficient on its own because nonce_account::verify_nonce_account only returns bool. so even with the hash checks swapped, if a transaction leads with a SystemProgram::AdvanceNonceAccount ix, but references some other rbh that's still extant in the queue, we fall back to the non-nonced tx check and the transaction is accepted.

i can pick this up in the morning

nguyenkha · 2022-06-02T07:57:43Z

Hi, is there any ETA for fix of nonce account? My software relies on the nonce account feature for durable transaction due to offline signing. Thank you.

Pull request has been modified.

@t-nelson

Durable nonce transactions can be executed twice. @t-nelson: > gist is to flip the nonce/non-nonce blockhash check so that whether a > durable nonce transaction is flagged is not dependent up whether the > blockhash is extant in the rbh queue

behzadnouri · 2022-06-02T14:48:19Z

sry, changing my mind. this change is insufficient on its own because nonce_account::verify_nonce_account only returns bool. so even with the hash checks swapped, if a transaction leads with a SystemProgram::AdvanceNonceAccount ix, but references some other rbh that's still extant in the queue, we fall back to the non-nonced tx check and the transaction is accepted.

i can pick this up in the morning

@t-nelson How about the last commit here?

The code is now distinguishing between cases where:

Transaction message does not refer to a durable nonce address.

Whereas transaction message is referring to a durable nonce address but:

Nonce account is not found in accounts-db.
Or, nonce account is invalid, i.e. verify_nonce_account returns false.
Or, durable nonce is disabled.

behzadnouri · 2022-06-02T15:50:25Z

Added a 2nd feature activation:

The 1st feature flips nonce/non-nonce blockhash check but keeps durable nonce disabled.
The 2nd feature enables durable nonce.

This should prevent double execution at epoch boundary when the 1st feature is activated.

t-nelson · 2022-06-02T18:04:28Z

runtime/src/bank.rs

+        &self,
+        message: &SanitizedMessage,
+    ) -> std::result::Result<TransactionAccount, NonceError> {
+        let nonce_address = message
            .get_durable_nonce(self.feature_set.is_active(&nonce_must_be_writable::id()))


I think we need to plumb NonceError through here and discern between the SystemProgram::AdvanceNonceAccount marker ix existing (nonced tx or not) and the referenced nonce account not existing (nonced tx that's going to fail)

t-nelson · 2022-06-02T18:04:28Z

runtime/src/bank.rs

+        let nonce_account = self
+            .get_account_with_fixed_root(nonce_address)
+            .ok_or(NonceError::NonceAccountNotFound)?;
+        if nonce_account::verify_nonce_account(&nonce_account, message.recent_blockhash()) {


I was considering plumbing NonceError through here too, so we can discern between an invalid nonce account and a blockhash mismatch, but I think it'd just be informational (and we'd eat that info immediately)

jstarry · 2022-06-02T22:04:25Z

Will this be closed in favor of #25744 ?

behzadnouri · 2022-06-03T17:09:10Z

Will this be closed in favor of #25744 ?

lets see if #25744 passes tests

jstarry · 2022-06-04T16:50:10Z

Quick question :) We're serializing durable nonce transactions using the blockhash we receive for a nonce account from the node. If I understand correctly, no need to change anything on my part even after this change, right? The new blockhash derivation will only change what the node returns and it is used as before.

We'll release guidance on this soon once the patch is finalized

maayank · 2022-06-04T16:55:22Z

Quick question :) We're serializing durable nonce transactions using the blockhash we receive for a nonce account from the node. If I understand correctly, no need to change anything on my part even after this change, right? The new blockhash derivation will only change what the node returns and it is used as before.

We'll release guidance on this soon once the patch is finalized

Thank you for the answer! I accidentally posted the question in this issue so deleted it and posted in the more recent issue.

behzadnouri added the noCI Suppress CI on this Pull Request label Jun 1, 2022

behzadnouri force-pushed the nonce-blockhash-check branch 4 times, most recently from 7fe8ae5 to 9b84bc5 Compare Jun 1, 2022

behzadnouri removed the noCI Suppress CI on this Pull Request label Jun 1, 2022

behzadnouri marked this pull request as ready for review Jun 1, 2022

behzadnouri force-pushed the nonce-blockhash-check branch from 9b84bc5 to 7e6abf2 Compare Jun 1, 2022

behzadnouri requested review from CriesofCarrots, jstarry, mvines, sakridge and t-nelson Jun 1, 2022

jstarry previously approved these changes Jun 1, 2022

View changes

behzadnouri force-pushed the nonce-blockhash-check branch from 7e6abf2 to bcab86a Compare Jun 1, 2022

CriesofCarrots previously approved these changes Jun 1, 2022

View changes

behzadnouri force-pushed the nonce-blockhash-check branch from bcab86a to 9f1cd5a Compare Jun 1, 2022

t-nelson previously approved these changes Jun 1, 2022

View changes

behzadnouri requested a review from ryoqun Jun 2, 2022

mvines added v1.9 labels Jun 2, 2022

t-nelson mentioned this pull request Jun 2, 2022

runtime: disable durable nonces #25696

Merged

t-nelson reviewed Jun 2, 2022

View changes

t-nelson self-requested a review Jun 2, 2022

behzadnouri force-pushed the nonce-blockhash-check branch from 9f1cd5a to 35cf451 Compare Jun 2, 2022

behzadnouri force-pushed the nonce-blockhash-check branch 2 times, most recently from 36c76ce to 1c3bf59 Compare Jun 2, 2022

flips nonce/non-nonce blockhash check

3361655

Durable nonce transactions can be executed twice. @t-nelson: > gist is to flip the nonce/non-nonce blockhash check so that whether a > durable nonce transaction is flagged is not dependent up whether the > blockhash is extant in the rbh queue

behzadnouri force-pushed the nonce-blockhash-check branch 4 times, most recently from adcd09e to d043479 Compare Jun 2, 2022

distinguishes between no nonce address and invalid nonce account

a3cff7e

behzadnouri force-pushed the nonce-blockhash-check branch from d043479 to a3cff7e Compare Jun 2, 2022

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

keeps nonce disabled even after flipping blockhash check

bb789da

t-nelson reviewed Jun 2, 2022

View changes

t-nelson mentioned this pull request Jun 2, 2022

separates durable nonce and blockhash domains #25744

Merged

solana-labs deleted a comment from RieserStern Jun 3, 2022

behzadnouri closed this in #25744 Jun 4, 2022

flips nonce/non-nonce blockhash check #25711

flips nonce/non-nonce blockhash check #25711

behzadnouri commented Jun 1, 2022 •

edited

jstarry left a comment

CriesofCarrots left a comment

t-nelson left a comment

t-nelson Jun 1, 2022

behzadnouri commented Jun 1, 2022

t-nelson commented Jun 2, 2022

t-nelson left a comment

nguyenkha commented Jun 2, 2022

behzadnouri commented Jun 2, 2022 •

edited

behzadnouri commented Jun 2, 2022 •

edited

t-nelson Jun 2, 2022

t-nelson Jun 2, 2022

jstarry commented Jun 2, 2022

behzadnouri commented Jun 3, 2022

jstarry commented Jun 4, 2022

maayank commented Jun 4, 2022

flips nonce/non-nonce blockhash check #25711

flips nonce/non-nonce blockhash check #25711

Conversation

behzadnouri commented Jun 1, 2022 • edited

Problem

Summary of Changes

jstarry left a comment

CriesofCarrots left a comment

t-nelson left a comment

t-nelson Jun 1, 2022

Choose a reason for hiding this comment

behzadnouri commented Jun 1, 2022

t-nelson commented Jun 2, 2022

t-nelson left a comment

nguyenkha commented Jun 2, 2022

behzadnouri commented Jun 2, 2022 • edited

behzadnouri commented Jun 2, 2022 • edited

t-nelson Jun 2, 2022

Choose a reason for hiding this comment

t-nelson Jun 2, 2022

Choose a reason for hiding this comment

jstarry commented Jun 2, 2022

behzadnouri commented Jun 3, 2022

jstarry commented Jun 4, 2022

maayank commented Jun 4, 2022

behzadnouri commented Jun 1, 2022 •

edited

behzadnouri commented Jun 2, 2022 •

edited

behzadnouri commented Jun 2, 2022 •

edited