-
Notifications
You must be signed in to change notification settings - Fork 97
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
SIMD-0064: Transaction Receipts #64
SIMD-0064: Transaction Receipts #64
Conversation
cc @samkim-crypto we would appreciate your review on this :) |
As requested, here are some thoughts on the implementation change: The detailed design part of this document does not fully specify the commitment scheme introduced. In its final form, the PR should minimally include:
I would further be interested in the following design considerations. These are part of a larger opportunity to optimize and standardize uses of hash trees in the protocol.
A further possibility for optimization is the hash function construction: SHA-256 has an internal block size of 64 bytes. By prefixing with a single byte, a branch node will have a 65 byte preimage, breaking message alignment across two blocks. It would be worth using a 64 byte prefix instead. A fast implementation would cache the SHA state after applying the first block, skipping one block of SHA hashing every branch. As performance is currently mostly bound by SHA-256 and sorting, this should result in a noticeable speedup. Instead of mentioning design properties in the security consideration section, I suggest structuring it into pairs of problem/solution. Here's a possible structure (Not complete, just for the sake of example)
When copying text, I suggest using quote blocks and including a link to the source. Before merging, it would also be nice to fix grammar/spelling (e.g. lowercase "rust" => "Rust") Finally, to reflect on design a bit: I would very much like to improve the status quo than repeat past mistakes (such as issues with the bank and PoH hash trees). However, the Firedancer is currently fairly busy with work on other parts on the runtime. Once we can dedicate some time to this, I intend to write up a specification and implementation including above performance improvements and security fixes. |
Terribly sorry for the delay in looking at this 🙏 . In my opinion, the proposal makes a lot of sense overall 👍 . @ripatel-fd has some really nice suggestions, so I think some of his points can be incorporated into the proposal. A popular way to prevent a length extension attack in this type of scenario where the length of the leaves is not fixed (and perhaps you want to leave signatures untouched) is to hash the root hash one more time with the length of the leaves.
where Some other comments:
|
For now, my preference would be to stay with SHA256, and only change hash functions if the change is made everywhere in the protocol. |
…signatures Removed signatures and added message hashes for our benchmarks.
Co-authored-by: Trent Nelson <[email protected]>
Co-authored-by: Trent Nelson <[email protected]>
Co-authored-by: Trent Nelson <[email protected]>
Co-authored-by: Trent Nelson <[email protected]>
Co-authored-by: Trent Nelson <[email protected]>
Update author list
Co-authored-by: Trent Nelson <[email protected]>
Fix hash notation
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.
last one and we're good by my eye
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.
@jacobcreech ball's in your court ⛹️♂️ |
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.
Requesting changes due to issue in the hash tree pseudocode.
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.
Approving on behalf of Firedancer
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.
Thanks for all the work @anoushk1234 @harsh4786 @ripatel-fd @t-nelson ! Good to see this come to consensus
* create transaction receipt simd * update * Update 0063-transaction-receipt.md * Update 0063-transaction-receipt.md * fix name * tree spec wip * receipt tree spec wip * fixup * remove logs from receipt * update * update * update * fix lint * fix bankhash * Update 0064-transaction-receipt.md * Update 0064-transaction-receipt.md * Update 0064-transaction-receipt.md * update tree spec * change receipt structure to use Message hash instead of a signature. * bench: add benchmarks on receipt tree with message hashes instead of signatures Removed signatures and added message hashes for our benchmarks. * minor fixes * minor fixes * Update 0064-transaction-receipt.md * fix: add len of receipts to tree * fix: add byte ordering for length suffix * fix: lint * change Receipt to TransactionReceipt Co-authored-by: Trent Nelson <[email protected]> * change slot to block Co-authored-by: Trent Nelson <[email protected]> * optimisations and clean up * remove redundant comment Co-authored-by: ripatel-fd <[email protected]> * remove redundant comment for version Co-authored-by: ripatel-fd <[email protected]> * add root for empty set. Co-authored-by: Richard Patel <[email protected]> * fix typo Co-authored-by: Richard Patel <[email protected]> * append justification for sha256 Co-authored-by: Richard Patel <[email protected]> * change terminology for receipts Co-authored-by: Richard Patel <[email protected]> * fix receipt terminology for tree Co-authored-by: Richard Patel <[email protected]> Co-authored-by: Trent Nelson <[email protected]> * Minor clean up * fix * Update 0064-transaction-receipt.md * grammar * clarify * typo * clarify * company name * precision * add layout and fix lint Co-authored-by: Richard Patel <[email protected]> * fix typo in transaction Co-authored-by: lheeger-jump <[email protected]> * make layout section more explicit Co-authored-by: Richard Patel <[email protected]> * nit: mention theoretical perf in hash function choice Co-authored-by: ripatel-fd <[email protected]> * change should to must - rf2119 Co-authored-by: Trent Nelson <[email protected]> * update should to must - rfc2119 Co-authored-by: Trent Nelson <[email protected]> * replace "fixed" with "avoided" to be more clear Co-authored-by: Trent Nelson <[email protected]> * More details in terminology section Co-authored-by: Trent Nelson <[email protected]> * fix lint and add missing node in tree spec Co-authored-by: Trent Nelson <[email protected]> * Clarify impact of receipts Co-authored-by: Trent Nelson <[email protected]> * fix lint * add intermediate_node when leaf count is zero Co-authored-by: Trent Nelson <[email protected]> * Author list * Fix hash notation Co-authored-by: Trent Nelson <[email protected]> * add empty intermediate root for empty vector illustration * Empty tree --------- Co-authored-by: harsh4786 <[email protected]> Co-authored-by: Trent Nelson <[email protected]> Co-authored-by: ripatel-fd <[email protected]> Co-authored-by: Richard Patel <[email protected]> Co-authored-by: lheeger-jump <[email protected]> Co-authored-by: Trent Nelson <[email protected]>
As Solana moving towards accounts hashing, will light clients be supported per this merkle trees based design? If not, what are the alternatives? |
Goal
Introduce the Receipt and Receipt Tree into the core protocol to allow for validating status of a confirmed transaction without trusting the RPC.
Notes
We've drafted this SIMD in collaboration with @ripatel-fd and have already discussed it with the Jump Firedancer team and @aeyakovenko. We would appreciate any feedback given to help move this SIMD forward.