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.

Sign up for GitHub

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

Jump to bottom

Add finalizer commits merkle root to block header #815

Merged
merged 10 commits into from
Mar 26, 2019
Merged

Add finalizer commits merkle root to block header #815

merged 10 commits into from
Mar 26, 2019

Conversation

kostyantyn
Copy link
Member

This PR adds merkle root of finalizer commits to the block header.
Having this merkle root in a header will allow us to verify
the consistency of headers+commits P2P messages.

Notice, original "hashMerkleRoot" also takes finalizer commits into account.
The reason is that it's needed for Bloom Filter. We can review
the possibility of not including finalizer commits into hashMerkleRoot
and refactor Bloom Filters/SPV clients in a separate issue or PR.

We also have an issue of how to optimize our merkle roots #806
and this will be discussed/tackled in a separate PR.

Signed-off-by: Kostiantyn Stepaniuk kostia@thirdhash.com

frolosofsky
frolosofsky reviewed Mar 20, 2019
View reviewed changes
src/txdb.cpp Show resolved Hide resolved
src/staking/block_validator.cpp Outdated Show resolved Hide resolved
src/test/merkle_tests.cpp Outdated Show resolved Hide resolved
src/test/merkle_tests.cpp Outdated Show resolved Hide resolved
src/test/merkle_tests.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
test/functional/test_framework/messages.py Show resolved Hide resolved
test/functional/test_framework/messages.py Show resolved Hide resolved
@frolosofsky
Copy link
Member

ConceptACK 0d64ed0

@scravy
Copy link
Member

scravy commented Mar 20, 2019

That crusade against commas is unnecessary code churn in my opinion.

scravy
scravy reviewed Mar 20, 2019
View reviewed changes
src/test/merkle_tests.cpp Outdated Show resolved Hide resolved
@kostyantyn kostyantyn force-pushed the add_commits_merkle_root_to_header branch from 19b28e1 to 0de215a Compare March 21, 2019 12:50
@kostyantyn
Copy link
Member Author

rebased with master and force-pushed because of the issue with unit tests

Gnappuraz
Gnappuraz previously approved these changes Mar 21, 2019
View reviewed changes
Copy link
Member

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

ConceptACK 1fc80f7, I wanna see where the comma argument settled before acking.

@Gnappuraz Gnappuraz self-requested a review March 21, 2019 14:36
@Gnappuraz Gnappuraz dismissed their stale review March 21, 2019 14:36

mistakenly approved

@kostyantyn kostyantyn force-pushed the add_commits_merkle_root_to_header branch from 1fc80f7 to 4214e90 Compare March 21, 2019 15:26
@kostyantyn
Copy link
Member Author

Force-pushed because had to resolve conflicts with master

@kostyantyn kostyantyn requested a review from a team March 22, 2019 09:07
AM5800
AM5800 reviewed Mar 22, 2019
View reviewed changes
src/test/merkle_tests.cpp Outdated Show resolved Hide resolved
@frolosofsky frolosofsky mentioned this pull request Mar 22, 2019
Merged
@kostyantyn kostyantyn force-pushed the add_commits_merkle_root_to_header branch from 55a6f4e to ecdc68b Compare March 22, 2019 12:44
@kostyantyn
Copy link
Member Author

rebased with master and force-pushed because we renamed IsFinalizationTransaction to IsFinalizerCommits

AM5800
AM5800 approved these changes Mar 22, 2019
View reviewed changes
Copy link
Member

@AM5800 AM5800 left a comment

Choose a reason for hiding this comment

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

utACK ecdc68b
Comma argument is too rocket science to me ;D

frolosofsky
frolosofsky approved these changes Mar 22, 2019
View reviewed changes
Copy link
Member

@frolosofsky frolosofsky left a comment

Choose a reason for hiding this comment

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

utACK ecdc68b

Gnappuraz
Gnappuraz approved these changes Mar 22, 2019
View reviewed changes
Copy link
Member

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

utACK ecdc68b

@frolosofsky frolosofsky mentioned this pull request Mar 25, 2019
Closed
4 tasks
Kostiantyn Stepaniuk added 3 commits March 25, 2019 17:47
Add finalizer commits merkle root to block header
7be9e6b 
This PR adds merkle root of finalizer commits to the block header.
Having this merkle root in a header will allow us to verify
the consistency of headers+commits P2P messages.

Notice, original "hashMerkleRoot" also takes finalizer commits into account.
The reason is that it's needed for Bloom Filter. We can review
the possibility of not including finalizer commits into hashMerkleRoot
and refactor Bloom Filters/SPV clients in a separate issue or PR.

We also have an issue how to optimize our merkle roots dtr-org/unit-e#806
and this will be discussed/tackled in a separate PR.

Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
Cleanups and fixes
731befa 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
improve is_finalizer_commit
1317e26 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
Kostiantyn Stepaniuk added 7 commits March 25, 2019 17:47
Remove unneeded parameter
0b96de2 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
Fix interface_rest.py
6ee34ba 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
Fix interface_zmq.py
8ca4609 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
fix typos
25df3bb 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
Rename IsFinalizationTransaction to IsFinalizerCommit
9d101ee 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
Update BLOCK_HEADER_LENGTH
54c421c 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
Post rebase fixes
2359426 
Signed-off-by: Kostiantyn Stepaniuk <kostia@thirdhash.com>
@kostyantyn kostyantyn force-pushed the add_commits_merkle_root_to_header branch from ecdc68b to 2359426 Compare March 25, 2019 17:10
@kostyantyn
Copy link
Member Author

had to rebase and force-push because of the conflict with master.

thothd
thothd approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@thothd thothd left a comment

Choose a reason for hiding this comment

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

utACK 2359426

@kostyantyn kostyantyn merged commit a8a002f into dtr-org:master Mar 26, 2019
@kostyantyn kostyantyn deleted the add_commits_merkle_root_to_header branch March 26, 2019 09:56
@frolosofsky frolosofsky mentioned this pull request Apr 8, 2019
Merged
frolosofsky added a commit that referenced this pull request Apr 9, 2019
@frolosofsky
Rely on commits in CBlockIndex (#911)
89e4210 
* Rely on commits in CBlockIndex

After commits merkle root added (#815) we can trust these commits
so that we can use them when restoring finalization state from disk.

It's the last chapter and it finally fixes #836.

Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scravy scravy scravy left review comments

@frolosofsky frolosofsky frolosofsky approved these changes

@thothd thothd thothd approved these changes

@Gnappuraz Gnappuraz Gnappuraz approved these changes

@AM5800 AM5800 AM5800 approved these changes

Assignees
No one assigned
Labels
consensus p2p
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kostyantyn @frolosofsky @scravy @AM5800 @Gnappuraz @thothd