[Solana][NONEVM-1028] Deduplicate accounts in execution #477
Conversation
PabloMansanet
force-pushed
the
deduplicate_accounts
branch
from
a92072b to
22c759c
Compare
There was a problem hiding this comment.
We need to ensure that the hash of the message is the same offchain and onchain. Saving space for those values in the extra args message means that the offchain code will need to hash the message with those values in, and then remove them to send the message through the contract. Then the program will need to do the same but the other way out, adding them on the hash.
If you feel that the saved space is worth it we could do this change, but if not it feels like a possible UX concern of the protocol, as all the nodes will need to sign a message with those values in, but then the message sent through is different.
|
CC @aalu1418 after our sync discussion yesterday on how to proceed with this 🙂 |
PabloMansanet
force-pushed
the
deduplicate_accounts
branch
3 times, most recently
from
741a4fc to
614f00b
Compare
There was a problem hiding this comment.
Let's keep the serialization of the accounts inside the hash, with this change the hash values should remain the same.
Can you add a test validating that when sending different accounts in remaining accounts to the ones in the hash then execution fails? So that way we can check that there is no override of the accounts
chains/solana/contracts/programs/ccip-router/src/instructions/v1/offramp.rs
Outdated
Show resolved
Hide resolved
chains/solana/contracts/programs/ccip-router/src/instructions/v1/offramp.rs
Outdated
Show resolved
Hide resolved
chains/solana/contracts/programs/ccip-router/src/instructions/v1/offramp.rs
Outdated
Show resolved
Hide resolved
chains/solana/contracts/programs/ccip-router/src/instructions/v1/offramp.rs
Outdated
Show resolved
Hide resolved
chains/solana/contracts/programs/ccip-router/src/instructions/v1/offramp.rs
Show resolved
Hide resolved
PabloMansanet
force-pushed
the
deduplicate_accounts
branch
3 times, most recently
from
66d5e3b to
5772fce
Compare
| sender: execution_report.message.sender.clone(), | ||
| data: execution_report.message.data.clone(), |
There was a problem hiding this comment.
Why did you need that here? Does it help with the memory usage?
There was a problem hiding this comment.
That's just because moving the verify_merkle_root later in the function causes a borrow of a partially moved value: without the clone, part of execution_report would've moved out here.
chains/solana/contracts/programs/ccip-router/src/instructions/v1/offramp.rs
Outdated
Show resolved
Hide resolved
| &execution_report.message.logic_receiver, | ||
| ctx.remaining_accounts.is_empty(), | ||
| ) { | ||
| let hashed_leaf = if should_execute_messaging(ctx.remaining_accounts, token_indexes) { |
chains/solana/contracts/programs/ccip-router/src/instructions/v1/offramp.rs
Outdated
Show resolved
Hide resolved
| token_indices: &[u8], | ||
| ) -> bool { | ||
| // The first entry in the accounts corresponds to a token, which means there is no logic receiver | ||
| let only_tokens = token_indices.first().map(|i| *i == 0).unwrap_or_default(); |
There was a problem hiding this comment.
does the unwrap_or_default defaults to false?
There was a problem hiding this comment.
Yep it does! So if there are no token indices, this would be false.
chains/solana/contracts/programs/ccip-router/src/instructions/v1/offramp.rs
Outdated
Show resolved
Hide resolved
Remove unneeded field Rework test comment Update test hash deduplicate logic receiver Fix execution check Address review comments Hash accounts separately Fix more tests Don't hash token accounts [CCIP-4938] Use map instead of slice in token price observations and outcome (#498) * check number of observed timestamps is at least 2f+1 * Change token price processor to observe feed tokens as a map instead of slice Using maps instead of slices make it sure that no duplicates can be added. Apply comment suggestions Co-authored-by: Agustina Aldasoro <[email protected]>
PabloMansanet
force-pushed
the
deduplicate_accounts
branch
from
78128c8 to
0574909
Compare
|
@agusaldasoro addressed your review comments and added the tests you mentioned, PTAL when you have a moment :) |
PabloMansanet
force-pushed
the
deduplicate_accounts
branch
from
9bdc2f2 to
b2938ab
Compare
|
No description provided.