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

Bitcoin header chain verification per network #1637

Merged
merged 34 commits into from
Dec 24, 2024

Conversation

yaziciahmet
Copy link
Contributor

Description

Implements different header chain verification for each type of bitcoin networks: mainnet, testnet4, signet, regtest.

The only work remaining is to decide on the initial DA blocks for mainnet, testnet4, and signet.

Linked Issues

Testing

Describe how these changes were tested. If you've added new features, have you added unit tests?

Docs

Describe where this code is documented. If it changes a documented interface, have the docs been updated?

@auto-assign auto-assign bot requested a review from ercecan December 22, 2024 22:13
@yaziciahmet yaziciahmet added the HOLD-MERGE PR is not draft but should not be merged yet label Dec 22, 2024
Copy link
Member

@eyusufatik eyusufatik left a comment

Choose a reason for hiding this comment

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

well done

crates/light-client-prover/src/da_block_handler.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 22, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 118 lines in your changes missing coverage. Please review.

Project coverage is 78.7%. Comparing base (3ef406b) to head (40def29).
Report is 1 commits behind head on nightly.

Files with missing lines Patch % Lines
crates/bitcoin-da/src/verifier.rs 71.6% 98 Missing ⚠️
crates/light-client-prover/src/da_block_handler.rs 0.0% 11 Missing ⚠️
...ereign-sdk/full-node/db/sov-db/src/schema/types.rs 0.0% 9 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/bitcoin-da/src/network_constants.rs 100.0% <100.0%> (ø)
crates/light-client-prover/src/circuit.rs 95.4% <100.0%> (+<0.1%) ⬆️
...tes/sovereign-sdk/adapters/mock-da/src/verifier.rs 84.6% <100.0%> (-0.3%) ⬇️
...sovereign-sdk/rollup-interface/src/node/rpc/mod.rs 88.9% <100.0%> (+<0.1%) ⬆️
...reign-sdk/rollup-interface/src/state_machine/da.rs 74.0% <100.0%> (+0.3%) ⬆️
...n-sdk/rollup-interface/src/state_machine/zk/mod.rs 93.7% <ø> (ø)
...ereign-sdk/full-node/db/sov-db/src/schema/types.rs 71.3% <0.0%> (-1.7%) ⬇️
crates/light-client-prover/src/da_block_handler.rs 0.0% <0.0%> (ø)
crates/bitcoin-da/src/verifier.rs 76.8% <71.6%> (+30.1%) ⬆️

... and 3 files with indirect coverage changes

@yaziciahmet
Copy link
Contributor Author

One thing I realized is that, we actually don't need total work. We just store it, and add to it, no checks whatsoever. Should we remove it?

Copy link
Collaborator

@ozankaymak ozankaymak left a comment

Choose a reason for hiding this comment

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

I implemented all functionalities in a single function, did not seperate according to the network since we can decide it in compile time, see the pull request and the file. It would also be a good double check for me.

crates/bitcoin-da/src/verifier.rs Outdated Show resolved Hide resolved
crates/bitcoin-da/src/verifier.rs Show resolved Hide resolved
@yaziciahmet
Copy link
Contributor Author

I implemented all functionalities in a single function, did not seperate according to the network since we can decide it in compile time, see the pull request and the file. It would also be a good double check for me.

Due to our current code structure, it is hard and ugly for us to decide this on compile-time. Also, this makes it a bit more testable with the unit and integration tests, not limiting us to e2e tests where we compile each target accordingly and test. Hence, I tried to get away with one match statement in runtime and no other overhead.

crates/bitcoin-da/src/verifier.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ozankaymak ozankaymak left a comment

Choose a reason for hiding this comment

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

Lgtm

@eyusufatik eyusufatik removed the HOLD-MERGE PR is not draft but should not be merged yet label Dec 24, 2024
@eyusufatik eyusufatik merged commit 1dde2ac into nightly Dec 24, 2024
14 of 15 checks passed
@eyusufatik eyusufatik deleted the yaziciahmet/bitcoin-difficulty-per-network branch December 24, 2024 11:14
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.

Bitcoin header chain for different networks
3 participants