Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add proposal for transactions v2 and address map program #17103
Add proposal for transactions v2 and address map program #17103
Changes from 3 commits
ae45cd4
9a5dabf
006608e
94d9a74
b77af6b
d29d11b
bdbc2c4
54d8fdf
ad42eb5
50dd231
03aaf4c
39e2cbd
70de74c
8a858f6
f76b243
d5db732
31d96c0
1a2432d
abada96
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about writing down bpf-visible changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also what about rpc, explorer, cli? should we probably abstract away this and present as if the account keys are just large at the rpc and ui? CC: @CriesofCarrots @oJshua
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryoqun yes, no bpf visible changes should happen. The instruction sysvar uses decompiled instructions which will not know anything about account indexes.
As for client side impact. I think that RPC should support returning both compressed and decompressed transactions. I wrote up details here: https://github.com/solana-labs/solana/pull/17103/files#diff-e15f9fcbfd6f181fc2a36cf56d0b5bc1d6837fbd1da1d88a5739d12f6878d549R116-R120
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe account index must be grouped by program id and authorized to the program id owner?
I came up with this attack (unrealistic though): if tx uses untrusted index account...:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, it will be a good practice for any wallets to check the actually referenced account address via index before signing like they're doing for account keys?
Otherwise, their signer bit can be abused covertly by referencing via cpi and the compromised account index account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also thought about hard bad possible interplay regarding nonces and gossiped vote transactions but I couldn't come up with anything particularly bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please elaborate a bit more here, I didn't quite understand this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we might want to add an integrity check. Perhaps we add the sha256 hash of all accounts referenced via indexes to the transaction message?
It might be overkill though. This is not really any different from a malicious actor convincing a user to use an account with some state A1 and then forking the chain to change the account to state A2. It's the client's responsibility to confirm the finality all account state before signing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think an integrity check could be done as a separate instruction. Users who desire more security can add an assertion instruction which hashes the indexed account addresses together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The effects of this indirection on what the transaction signatures are saying has been bothering me a bit as well. Ideally, we'd sign over the actual addresses, use the indices for broadcast, then recover the addresses before sigverify. I don't think this is practical though.
Alternatively, we could add another field of
{indexed_addresses_witness_index: u8, indexed_addresses_signature: Signature}
, that's checked at runtime between accounts load and transaction execution. This should be robust if we require that theaddress_witness
has also signed the broadcast encoding. It'll cost us 65 bytes though.Theoretically, this would be vulnerable to a birthday attack unless the program had full control of the hash. We can mitigate by mixing in date outside an attackers control, like the most recent blockhash. It would be pretty costly to the size of the index entries though
Attaching an
insertion_slot
to each index and requiring a cool down period before use would likely be sufficientI'm apt to agree. It seems like these attacks need sufficient control over the network as to assume it's compromised in many other ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my concern is no longer valid with these bits :) :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a paragraph about the attack vector where indexes are modified pre-finalization. I think we can just recommend that clients wait for finalization before using an index. They can also add their own integrity checks if needed