Skip to content
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

Added support for withdrawals #4

Merged
merged 9 commits into from
Jan 22, 2024
Merged

Added support for withdrawals #4

merged 9 commits into from
Jan 22, 2024

Conversation

BGluth
Copy link
Contributor

@BGluth BGluth commented Dec 1, 2023

Two part PR. The second PR will be upstream in zero-bin.

@BGluth BGluth requested a review from Nashtare December 1, 2023 17:23
Copy link
Contributor

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. As discussed offline, we'll need to possibly update this or plonky2's logic at some point.

@Nashtare
Copy link
Contributor

Nashtare commented Dec 1, 2023

Actually @BGluth we may want to think on how to handle from now. I think the reason why we're processing withdrawals if there's no txn is to amortize the cost (cc @wborgeaud if I'm wrong). Then we may want to pass an empty vec throughout the entire set of txns, and then always append a dummy proof (in the sense no txn), with the withdrawals, if there are any.
That way:

  • empty blocks aren't impacted, as we keep the withdrawals vector empty
  • partially empty blocks (1 dummy + 1 txn) and full blocks (2+ txns), may have a light overhead of possibly requiring an additional proof at the end, with no signed_txn and the list of withdrawals, if it isn't empty. Does that sound alright to you?

@wborgeaud
Copy link

Actually @BGluth we may want to think on how to handle from now. I think the reason why we're processing withdrawals if there's no txn is to amortize the cost (cc @wborgeaud if I'm wrong). Then we may want to pass an empty vec throughout the entire set of txns, and then always append a dummy proof (in the sense no txn), with the withdrawals, if there are any

I guess it depends on the number of withdrawals. Like if there's only one, it might be cheaper to just add the withdrawal to the last txn instead of having an additional proof with an empty txn.
On that topic, the Plonky2 logic is a bit unsound since we should check that the list of withdrawals is empty when a txn is not the last in the block. (The whole withdrawals logic is unsound tbh since we don't check the withdrawals root, but I guess it's good to keep that in mind)

@BGluth
Copy link
Contributor Author

BGluth commented Dec 4, 2023

Ah ok, so maybe just hold off on this PR for now then?

@Nashtare
Copy link
Contributor

Nashtare commented Dec 4, 2023

We'll need this soon though, if we want to prove real blocks :) But up to you!

@Nashtare
Copy link
Contributor

Nashtare commented Dec 6, 2023

@BGluth As we'll need withdrawals to test real-life block, I'd suggest updating the logic to include always a dummy txn for now at the end of a block, after the last actual txn, containing no txn but the full withdrawals vector.

For partially empty blocks we would then have:

  • no difference if there is no withdrawal in the block
  • a fully dummy payload and a dummy (no txn) but with the withdrawals vec for the second one in case of a fully empty block with withdrawals
  • no more dummy + actual txn but actual txn + dummy payload with withdrawals in case a block contains 1 txn only and withdrawals.

We can always update / optimize this logic in the future, as William mentioned we could always add them to the last txn if there aren't many, but that will require updating plonky2 anyway, so could be left for future work.

@BGluth
Copy link
Contributor Author

BGluth commented Dec 6, 2023

Sounds good. I'll update this PR and then give you a ping when it's ready for a review.

@dvdplm
Copy link
Contributor

dvdplm commented Jan 10, 2024

Is this PR still relevant?

@Nashtare
Copy link
Contributor

@dvdplm Yeah, we've been busy with the debugging the whole e2e flow but we still need to support withdrawals

@BGluth
Copy link
Contributor Author

BGluth commented Jan 18, 2024

Alright I'm going to try to get this done this afternoon. I don't think I made the suggested changed above, so I'll do that now and add @Nashtare as a reviewer.

@BGluth
Copy link
Contributor Author

BGluth commented Jan 19, 2024

Ok good to go! Let me know if this is works.

Copy link
Contributor

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I wrote some nitpicky comments inline. However, I think we still need some changes on the dummy inputs generation:

  • trie_roots_after for dummy proofs are constructed as:
    let trie_roots_after = TrieRoots {
        state_root: tries.state_trie.hash(),
        transactions_root: EMPTY_TRIE_HASH,
        receipts_root: EMPTY_TRIE_HASH,
    };

This is valid for padding dummy txns, which are always prepended to the list of actual txns in a block, but this won't be for a dummy txn to be created after the entire block execution to accomodate for withdrawals

  • The state_root component of tries_root_after will also need updating for the dummy_txn containing the withdrawals, as these update the state.

protocol_decoder/src/decoding.rs Outdated Show resolved Hide resolved
protocol_decoder/src/decoding.rs Outdated Show resolved Hide resolved
protocol_decoder/src/decoding.rs Outdated Show resolved Hide resolved
protocol_decoder/src/decoding.rs Outdated Show resolved Hide resolved
protocol_decoder/src/processed_block_trace.rs Outdated Show resolved Hide resolved
protocol_decoder/src/decoding.rs Outdated Show resolved Hide resolved
@BGluth BGluth requested a review from Nashtare January 20, 2024 04:22
Copy link
Contributor

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's still an issue for the case where dummy txns were already added (but state post withdrawal isn't)

protocol_decoder/src/decoding.rs Outdated Show resolved Hide resolved
protocol_decoder/src/decoding.rs Show resolved Hide resolved
@BGluth BGluth requested a review from Nashtare January 22, 2024 17:13
@BGluth BGluth requested a review from Nashtare January 22, 2024 17:42
protocol_decoder/src/decoding.rs Outdated Show resolved Hide resolved
protocol_decoder/src/decoding.rs Show resolved Hide resolved
@BGluth BGluth requested a review from Nashtare January 22, 2024 18:03
Copy link
Contributor

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@BGluth BGluth merged commit fc37719 into main Jan 22, 2024
2 checks passed
@BGluth BGluth deleted the withdrawals branch January 22, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants