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

state proofs: add block hash to LightBlockHeader #5663

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

zeldovich
Copy link
Contributor

Replace the Seed value in the state proof light header with BlockHash. It serves a similar purpose in terms of the second-order consideration of defending against potential future hash-collision attacks (albeit a little bit weaker), but has the benefit of allowing validating the entire blockchain, by committing to the block hash itself in the state proof.

Replace the `Seed` value in the state proof light header with `BlockHash`.
It serves a similar purpose in terms of the second-order consideration
of defending against potential future hash-collision attacks (albeit a
little bit weaker), but has the benefit of allowing validating the entire
blockchain, by committing to the block hash itself in the state proof.
cpeikert
cpeikert previously approved these changes Aug 14, 2023
Copy link

@cpeikert cpeikert left a comment

Choose a reason for hiding this comment

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

This looks good to me with respect to the cryptographic considerations Nickolai and I discussed (namely, replacing Seed with BlockHash in LightBlockHeader).

@cce cce changed the title add block hash to state proof state proofs: add block hash to state proof Aug 15, 2023
@cce cce changed the title state proofs: add block hash to state proof state proofs: add block hash to LightBlockHeader Aug 15, 2023
cce
cce previously approved these changes Aug 15, 2023
@cce
Copy link
Contributor

cce commented Aug 15, 2023

TestStateProofMessageCommitmentVerification is failing, which is an E2E test that looks like it spins up using vFuture

The test set a configurable consensus protocol, "test-fast-stateproofs",
and propagated the configuration to the child algod processes spawned
by the test, but did not update the supported consensus params in
the test process itself.  As a result, when the test code tried to
compute the light block header, it didn't have the block protocol
(test-fast-stateproofs) in its config.Consensus.
@zeldovich zeldovich dismissed stale reviews from cce and cpeikert via 55e05b9 August 15, 2023 15:01
@zeldovich
Copy link
Contributor Author

Looks like a slight oversight in TestStateProofMessageCommitmentVerification that didn't matter until this change. Let's see if the fix I just pushed fixes the e2e test in CI (it does fix it on my machine).

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (90b10d2) 55.81% compared to head (83f5c0f) 55.79%.

Files Patch % Lines
config/consensus.go 16.66% 5 Missing ⚠️
data/bookkeeping/lightBlockHeader.go 57.14% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5663      +/-   ##
==========================================
- Coverage   55.81%   55.79%   -0.03%     
==========================================
  Files         476      476              
  Lines       67138    67146       +8     
==========================================
- Hits        37476    37464      -12     
- Misses      27141    27152      +11     
- Partials     2521     2530       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zeldovich
Copy link
Contributor Author

Corresponding go-algorand-sdk change: algorand/go-algorand-sdk#589

@Priegle
Copy link
Contributor

Priegle commented Aug 16, 2023

Corresponding go-algorand-sdk change: algorand/go-algorand-sdk#589

do the other sdks need updates as well?

@zeldovich
Copy link
Contributor Author

do the other sdks need updates as well?

Probably so, yes. Nobody is likely to be using the light block headers (which gained an extra field), but it might be that some users will be looking at the protocol config values (which also gained an extra field). What have we done in the past when adding new protocol configuration parameters to ConsensusParams?

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

LGTM

@algorandskiy algorandskiy merged commit cd97193 into algorand:master Nov 17, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants