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

feat: not inclusive start block #68

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

invocamanman
Copy link
Contributor

@invocamanman invocamanman commented Feb 25, 2025

Description

We use the start_block as the first block that's proved. That would mean that the block proven follows the [start_block, finalBlock] convention.

Usually in rollups is used the (start_block, finalBlock] convention, so, start block not inclusive.
This is useful usually to check that the last finalBlock matches the start_block of the next proving block.

As an example of our current and new approach given a scenario with 2 range proofs. The first proof proves the block 1 and 2 and the second one proves the block 3:

Current Approach

Proof start_block end_block
Proof 1 1 2
Proof 2 3 3

New Convention

Proof start_block end_block
Proof 1 0 2
Proof 2 2 3

Additions and Changes

I fix a condition and add comments regarding this change
Mainly i think this will be relevant in the upcoming PRs.

For example now instead of doing this kinda minus one scenarios we will be able to use start_block right away

It's similar to the common convention with the rollups and personally i find it more intuitive ( maybe bc i work on a rollup for a while :P)

PR Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added or updated tests that comprehensively prove my change is effective or that my feature works

@atanmarko
Copy link
Contributor

atanmarko commented Feb 25, 2025

@invocamanman I find this very confusing.
It makes more sense that start_block..end_block gets into the proof (inclusive).

EDIT - if this is how other count proof ranges, then ok.

@invocamanman invocamanman marked this pull request as ready for review February 25, 2025 21:53
@invocamanman invocamanman requested a review from a team as a code owner February 25, 2025 21:53
@invocamanman invocamanman force-pushed the feat/non-inclusive-start-block branch from 2a3791b to 3d81cb9 Compare February 25, 2025 22:01
atanmarko
atanmarko previously approved these changes Feb 26, 2025
@invocamanman invocamanman changed the title Not inclusive start block feat: not inclusive start block Feb 26, 2025
@invocamanman invocamanman force-pushed the feat/non-inclusive-start-block branch from 3d81cb9 to e7ea0f1 Compare February 26, 2025 13:41
@invocamanman invocamanman force-pushed the feat/non-inclusive-start-block branch from e7ea0f1 to 93ac82c Compare February 26, 2025 13:43
@invocamanman invocamanman force-pushed the feat/non-inclusive-start-block branch from 93ac82c to 650adc6 Compare February 26, 2025 14:18
@invocamanman invocamanman force-pushed the feat/non-inclusive-start-block branch 2 times, most recently from f19bb03 to 0720b14 Compare February 26, 2025 15:11
@atanmarko atanmarko self-requested a review February 26, 2025 15:17
@invocamanman invocamanman force-pushed the feat/non-inclusive-start-block branch from 0720b14 to 55a6e30 Compare February 26, 2025 15:19
Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

Wondering why was this convention introduced, as it's quite different from the usual [start, end) convention which may cause unintended off-by-one errors.

@Freyskeyd
Copy link
Member

How does it integrate with the aggsender ? cc @arnaubennassar
And what's about the fer-proposer ? cc @vcastellm

@goran-ethernal
Copy link

How does it integrate with the aggsender ? cc @arnaubennassar And what's about the fer-proposer ? cc @vcastellm

In regards to the aggsender, we are using approach 1 (the old approach). One question I have about this:

  • if we are going to use the new approach, does that mean we will not be sending the merkle proofs, imported bridge exits, and what not, for the start block?

Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@hadjiszs
Copy link
Contributor

if we are going to use the new approach, does that mean we will not be sending the merkle proofs, imported bridge exits, and what not, for the start block?

yeah I believe no witness for the start block because they would have already been provided as part of the end block of the previous request

@arnaubennassar
Copy link

IMO it makes more sense to be inclusive, it's more intuitive... If I request a proof from block 10 to 20, I expect a proof from block 10 to 20, not from 11 to 20 😅

@Freyskeyd
Copy link
Member

For this subject, after discussing with different members, it appears that we've not an agreement on this.

Question: @vcastellm, I remember when discussing this with you that the fep-proposer is expecting an exclusive range of block number, right?
Meaning that the fep-proposer is expecting to act like:

Received Returned
0 ... 10 1 ... 10

To do some pros and cons, for both solution:

Inclusive

As @arnaubennassar said, when reading the API, it is expected that the caller receive a proof from start...end.

Pros

  • Clear from the caller perspective that the received proof is starting at start
  • Common practice in general

Cons

  • The aggkit-prover needs -1 the start in order to fetch the previous LER etc
  • The fields received by the aggkit-prover will not match the one sent to the fep-proposer

Exclusion

Pros

  • Same parameters from aggkit-prover to fep-proposer, easier to debug
  • Common practice in rollups

Cons

  • The aggkit-prover needs +1 the start after fetching the previous LER etc
  • Less clear for the caller as it'll receive a proof starting from start + 1

I think both options have good and valid points, something that can be a third option is to refactor how we think the API.
Instead of defining: "Give me a proof starting at this block up to this block if possible"
Why not define this as: "Here's the last block that has been proven, give me a proof up to this block if possible"

The change is how we define the start:

Current

// The request message for generating aggchain proof.
message GenerateAggchainProofRequest {
  // The start block for which the aggchain proof is requested.
  uint64 start_block = 1;
  // The max end block for which the aggchain proof is requested.
  uint64 max_end_block = 2;
  ...
}

Updated

// The request message for generating aggchain proof.
message GenerateAggchainProofRequest {
  // The latest proven block
  uint64 lastest_proven_block = 1;
  // The max end block for which the aggchain proof is requested.
  uint64 max_end_block = 2;
  ...
}

What do you think?

cc @invocamanman @arnaubennassar @vcastellm @agglayer/agglayer-developers

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.

8 participants