Add Message::is_maybe_writable
#35340
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35340 +/- ##
=======================================
Coverage 81.7% 81.7%
=======================================
Files 834 834
Lines 224299 224307 +8
=======================================
+ Hits 183361 183376 +15
+ Misses 40938 40931 -7 |
sdk/src/transaction/mod.rs
Outdated
| @@ -1074,7 +1074,7 @@ impl Transaction { | |||
| } | |||
| } | |||
|
|
|||
| pub fn uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> { | |||
| pub fn maybe_uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> { | |||
There was a problem hiding this comment.
Thinking about this one more, I wonder if we can come up with a more descriptive fn name; the returned ix definitely does use durable nonce but the nonce account may not be writable.
Also, I quibble with "uses" in the first place, since this is a method that returns the instruction, not a bool.
Super wordy, but what about try_get_durable_nonce_instruction? Sort of ignores the "maybe writable" part
There was a problem hiding this comment.
Yeah it definitely looks like it's using a durable nonce but in the context of the Solana runtime, a tx is only considered to be using a durable nonce if it's a "valid" durable nonce which requires the nonce account to be writable. Maybe a name with "valid" in it would be more clear?
There was a problem hiding this comment.
Yeah, fair. I'm open to adding "valid" somehow
There was a problem hiding this comment.
Actually I think I was focusing too much on the validity... a nonce is also only valid if the transaction's recent blockhash is correct. This method should probably not be doing the writable check at all.
I'm inclined to go with your suggestion of try_get_durable_nonce_instruction but I will add it as an alias and deprecate the original function because I really shouldn't have introduced that breaking change in the first place.
There was a problem hiding this comment.
Ok actually I think I'm going to just leave everything as is but take out the is-writable check from this function
sdk/src/transaction/versioned/mod.rs
Outdated
| @@ -189,7 +189,7 @@ impl VersionedTransaction { | |||
| /// instruction. Since dynamically loaded addresses can't have write locks | |||
| /// demoted without loading addresses, this shouldn't be used in the | |||
| /// runtime. | |||
| pub fn uses_durable_nonce(&self) -> bool { | |||
| pub fn maybe_uses_durable_nonce(&self) -> bool { | |||
There was a problem hiding this comment.
Same comment here about the fact that it definitely uses durable nonce, just is maybe writable. I haven't thought of a better option for this one, though.
|
automerge label removed due to a CI failure |
mergify
bot
removed
the
automerge
|
automerge label removed due to a CI failure |
mergify
bot
removed
the
automerge
|
@CriesofCarrots I just removed some tests that were checking for writable condition in |
Problem
After adopting a dynamic set of reserved account keys which are not allowed to be writable by transactions, it will not longer be possible to statically determine if a legacy message's accounts are writable or not. This is similar to how the accounts in versioned txs (which make use of address lookup tables) also can't be statically determined to be writable or not until the addresses are loaded from lookup tables. Versioned transaction messages expose a
is_maybe_writablemethod for this reason.There are a few places where the dynamic set of reserved account keys will not be available:
Summary of Changes
Add an
is_maybe_writablemethod toMessagewhich current has the same signature asis_writable. After #34901,Message::is_writablewill be changed to accept an additional parameter for the dynamic set of reserved account keys.Change the implementation of
uses_durable_nonceto no longer check whether the nonce account is writable. That method should never have added that validity check in the first place.Fixes #