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

IF: Add block extension to include latest commit QC known to proposer #1586

Closed
Tracked by #1508
arhag opened this issue Aug 31, 2023 · 10 comments · Fixed by #2011 or #2090
Closed
Tracked by #1508

IF: Add block extension to include latest commit QC known to proposer #1586

arhag opened this issue Aug 31, 2023 · 10 comments · Fixed by #2011 or #2090
Assignees

Comments

@arhag
Copy link
Member

arhag commented Aug 31, 2023

Proposer would take the latest irreversible HotStuff proofs (the latest block proposal with the final_on_qc pointing to an irreversible block along with QC for it) known to it and include it in a new block extension for the block that it constructs. This would only be the irreversible HotStuff proofs which means they would always be relevant for advancing LIB on the irreversible blockchain.

@enf-ci-bot enf-ci-bot moved this to Todo in Team Backlog Aug 31, 2023
@arhag arhag changed the title IF: Add block extension to include latest QC known to proposer IF: Add block extension to include latest commit QC known to proposer Aug 31, 2023
@heifner
Copy link
Member

heifner commented Aug 31, 2023

Add to block extension signed_block.block_extensions the latest vector of [hs_proposal_message, quorum_certificate_message] for a committed hs_proposal. This irreversible hs proof will be used during syncing to advance LIB.

The accepted block signal can be processed by qc_chain to move LIB if they are relevant. They will not be relevant if you are not syncing as they will likely be behind current. (Do we need to optimize this or can qc_chain quickly tell?). Either way qc_chain will advance LIB as it normally would, which is still to be determined #1522.

On finalize of pending block the controller will request all "required" commitments greater than its current from qc_chain. qc_chain is required to store them until LIB (including those that contribute to LIB). The controller will place them in the block extension and also in pending block state. They should be "pulled forward" by pending block state so any unwinding of blocks also unwinds the known and reported committed values. Note this is needed so that we do not need to repeat commitment values in block extension.

It is the responsibility of qc_chain to provide us all relevant commitment pairs from current commitment pairs. Relevant here can be sparse but must include finalizer set changes and proofs needed to validate the finalizer set changes.

During block validation the qc_chain will be consulted to validate provided commitment pairs. Failure to validate the pair will be considered by the block validation node as an invalid block. This validation can happen in parallel to block validation if expensive.

@arhag arhag added 👍 lgtm and removed triage labels Aug 31, 2023
@BenjaminGormanPMP BenjaminGormanPMP moved this from Todo to In Progress in Team Backlog Aug 31, 2023
@greg7mdp
Copy link
Contributor

greg7mdp commented Sep 6, 2023

@heifner @arhag I have a couple questions:

  1. why do we want this QC in a block extension, while we want the finalizer_set in the block_header extension?
  2. comment above suggests to add "all required commitments" to the block extension. I thought only the latest one was needed.

@heifner
Copy link
Member

heifner commented Sep 6, 2023

  1. The block_header extension is included in the digest that is the block_id. It needs to be information that can be deterministically verified. The QC info in the block extension is not deterministic. That is fine for a block extension, but not for a block_header extension.

  2. You need the one at time of finalizer_set change as well as periodically.

@greg7mdp
Copy link
Contributor

greg7mdp commented Sep 6, 2023

It needs to be information that can be deterministically verified.

I'm not quite understanding why the QC is not deterministic, while the hs_finalizer_set_extension that we include in block_header extensions is?

You need the one at time of finalizer_set change as well as periodically.

Why? I thought that as long as we knew what finalizer_set was active when the QC was signed, that would be enough to verify the QC and then advance the LIB.

@heifner
Copy link
Member

heifner commented Sep 6, 2023

It needs to be information that can be deterministically verified.

I'm not quite understanding why the QC is not deterministic, while the hs_finalizer_set_extension that we include in block_header extensions is?

The QC that any individual node has at any given time is according to network conditions; it is not deterministic on all nodes.

You need the one at time of finalizer_set change as well as periodically.

Why? I thought that as long as we knew what finalizer_set was active when the QC was signed, that would be enough to verify the QC and then advance the LIB.

Hmm, that does sound right to me. @arhag ?

@greg7mdp
Copy link
Contributor

greg7mdp commented Sep 6, 2023

The QC that any individual node has at any given time is according to network conditions; it is not deterministic on all nodes.

But every node should be able to verify that this is a valid commit QC signed by a Quorum of finalizers. Wouldn't that be enough?

@heifner
Copy link
Member

heifner commented Sep 6, 2023

The QC that any individual node has at any given time is according to network conditions; it is not deterministic on all nodes.

But every node should be able to verify that this is a valid commit QC signed by a Quorum of finalizers. Wouldn't that be enough?

Not for the block_header extension which is included in the block_id. That needs to be deterministically verified that all validators get the exact same digest.

@arhag
Copy link
Member Author

arhag commented Nov 8, 2023

Will revisit after #1881. May throw this issue away and create a new one. Or we may simply update this to reflect changes from #1881.

@arhag arhag added discussion and removed 👍 lgtm labels Nov 8, 2023
@arhag
Copy link
Member Author

arhag commented Dec 15, 2023

The rule for adding a QC as a block extension is as follows:

  1. The QC to consider is the one referenced by last_qc_block_num in the header extension (See: IF: Unification: Add block header extension #1911).
  2. No block extension for the QC should be added if that QC is not needed. Determine if not needed using the is_needed helper function on the parent block's block_header_state (See: IF: Unification: Create new block_header_state and block_state #1941).
  3. At most one QC is added as a block extension.

@arhag arhag moved this from Blocked to Todo in Team Backlog Dec 18, 2023
@arhag arhag assigned linh2931 and unassigned greg7mdp Dec 18, 2023
@linh2931 linh2931 moved this from Todo to In Progress in Team Backlog Dec 19, 2023
@linh2931 linh2931 moved this from In Progress to Awaiting Review in Team Backlog Jan 3, 2024
@BenjaminGormanPMP BenjaminGormanPMP moved this from Awaiting Review to Done in Team Backlog Jan 4, 2024
@arhag arhag reopened this Jan 5, 2024
@github-project-automation github-project-automation bot moved this from Done to Todo in Team Backlog Jan 5, 2024
@arhag arhag moved this from Todo to Blocked in Team Backlog Jan 5, 2024
@arhag
Copy link
Member Author

arhag commented Jan 5, 2024

Partially done, but to complete we must first wait until at least #2034 is complete and it would probably be best to wait until #2046 is also complete.

@arhag arhag moved this from Blocked to In Progress in Team Backlog Jan 10, 2024
@linh2931 linh2931 moved this from In Progress to Awaiting Review in Team Backlog Jan 16, 2024
@linh2931 linh2931 moved this from Awaiting Review to Done in Team Backlog Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment