feat: proofClaim in Rollup by just-mitch · Pull Request #8636 · AztecProtocol/aztec-packages

just-mitch · 2024-09-19T01:34:24Z

Add the proofClaim to the rollup.

Update the canPrune logic to account for it.

update pruning logic.

just-mitch · 2024-09-19T01:35:50Z

+  }
+
  function _prune() internal {
+    delete proofClaim;


otherwise it will be there when we reorg

When reached in a call to prune (outside of proposals) this will incur higher cost than something like the approach we discussed with the forced inclusion. Here it is fix size, and should overall make it slightly cheaper when happening in the intended flow, e.g., as part of proposing.

just-mitch · 2024-09-19T01:36:18Z

@@ -44,23 +44,29 @@ library Errors {
  error Outbox__BlockNotProven(uint256 l2BlockNumber); // 0x0e194a6d

  // Rollup


sorted alphabetically

LHerskind

Did not read through the full test, but here are some comments for now.

LHerskind · 2024-09-19T08:43:34Z

  uint256 public constant TIMELINESS_PROVING_IN_SLOTS = 100;

+  uint256 public constant CLAIM_DURATION_IN_L2_SLOTS = 13;
+  uint256 public constant PROOF_COMMITMENT_MIN_BOND_AMOUNT_IN_TST = 1000;


For the sake of sanity we should probably shove this into a constants file, since we will likely be needing it in the node as well, so having it in one place should help us a bit there.

LHerskind · 2024-09-19T08:46:59Z

+  }
+
  function _prune() internal {
+    delete proofClaim;


When reached in a call to prune (outside of proposals) this will incur higher cost than something like the approach we discussed with the forced inclusion. Here it is fix size, and should overall make it slightly cheaper when happening in the intended flow, e.g., as part of proposing.

LHerskind · 2024-09-19T08:48:41Z

    vkTreeRoot = _vkTreeRoot;
  }

+  function claimEpochProofRight(DataStructures.EpochProofQuote calldata _quote)


I'm not sure I like the claim name, mainly as claiming usually is claiming rewards or the like, but here your claim is practically "reducing" your balance because of the stake.

Yeah it's a tricky thing to name, since the proposer is the one doing it, but the provers bond is the one getting staked. Open to suggestions.

LHerskind · 2024-09-19T08:56:28Z

+    address currentProposer = getCurrentProposer();
+    uint256 epochToProve = getEpochToProve();
+
+    if (currentProposer != address(0) && currentProposer != msg.sender) {


If currentProposer == address(0) you don't have a committee, does it become weird with epochs then? At that point issue is essentially that sure you can have an epoch, but as anyone could propose into it, it seems likely that you would not end up being able to rely on it, so seems more likely that it would be the "proposeAndProof" at the same time similar to the fallback. Might just be the sanest to be in fallback if there is no committee actually.

Might just be the sanest to be in fallback if there is no committee actually.

I think I agree with this. We'll see how this needs to change in response when based fallback lands.

LHerskind · 2024-09-19T09:00:52Z

+    }
+
+    if (currentSlot % Constants.AZTEC_EPOCH_DURATION >= CLAIM_DURATION_IN_L2_SLOTS) {
+      revert Errors.Rollup__NotInClaimPhase(


I think a more useful error message would likely be:

error Rollup__NotInClaimPhase(uint256 slotInEpoch, uint256 claimSlotsInEpoch);

Reasoning being that you then get information that is more useful for debugging without needing the figure out what the constant is. This way if you get 13, 13 you know instantly that it was sent too late and you know it must be within the first 13 slots so you were 1 off.

LHerskind · 2024-09-19T09:50:01Z

@@ -53,6 +53,9 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
  // @todo  #8018
  uint256 public constant TIMELINESS_PROVING_IN_SLOTS = 100;


This one is kinda redundant with your stuff just below 🤷

LHerskind · 2024-09-19T11:18:33Z

+      revert Errors.Rollup__QuoteExpired(currentSlot, _quote.validUntilSlot);
+    }
+
+    address bondProvider = PROOF_COMMITMENT_ESCROW.stakeBond(_quote);


You stake, but there won't ever be an unstake. Bye bye money. See comment in other pr.

I was waiting to unstake the bond until the proof of epoch landed.

I filed #8652.

Fix #8608 Add the proofClaim to the rollup. Update the `canPrune` logic to account for it.

just-mitch added 2 commits September 18, 2024 15:52

add claimEpochProofRight to rollup.

f39c776

update pruning logic.

add tests and squash bugs

1ce624c

just-mitch requested review from LHerskind and Maddiaa0 September 19, 2024 01:34

just-mitch changed the title ~~Mt/8608-track-proofclaim-in-rollup~~ feat: proofClaim in Rollup Sep 19, 2024

just-mitch commented Sep 19, 2024

View reviewed changes

Comment thread l1-contracts/test/Rollup.t.sol Outdated

just-mitch mentioned this pull request Sep 19, 2024

feat: prune if needed #8617

Merged

LHerskind reviewed Sep 19, 2024

View reviewed changes

just-mitch added 2 commits September 19, 2024 12:24

break out tests

17e61d2

cleanup

f439c7a

just-mitch merged commit 77cdb19 into mt/8572-prune-if-needed Sep 20, 2024

just-mitch deleted the mt/8608-track-proofclaim-in-rollup branch September 20, 2024 00:19

just-mitch added a commit that referenced this pull request Sep 20, 2024

feat: proofClaim in Rollup (#8636)

6b1a7ba

Fix #8608 Add the proofClaim to the rollup. Update the `canPrune` logic to account for it.

just-mitch added a commit that referenced this pull request Sep 27, 2024

feat: proofClaim in Rollup (#8636)

caad4ff

Fix #8608 Add the proofClaim to the rollup. Update the `canPrune` logic to account for it.

		@@ -44,23 +44,29 @@ library Errors {
		error Outbox__BlockNotProven(uint256 l2BlockNumber); // 0x0e194a6d

		// Rollup

		@@ -53,6 +53,9 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
		// @todo #8018
		uint256 public constant TIMELINESS_PROVING_IN_SLOTS = 100;

Conversation

just-mitch commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

just-mitch commented Sep 19, 2024 •

edited

Loading