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 proof block height verification #400

Merged
merged 5 commits into from
Jul 19, 2023
Merged

Add proof block height verification #400

merged 5 commits into from
Jul 19, 2023

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Jul 13, 2023

Closes #387

@ffranr ffranr force-pushed the add_proof_block_height_verify branch from ff16d31 to 05289f7 Compare July 17, 2023 14:28
@ffranr ffranr force-pushed the add_proof_block_height_verify branch from 05289f7 to bad914e Compare July 17, 2023 18:56
@ffranr
Copy link
Contributor Author

ffranr commented Jul 17, 2023 •
edited
Loading

As far as I can tell, we should be able to release after this PR is merged.

Once this PR is merged, the header verifier takes into account both the block header and block height. Therefore, we should probably generalise the verifier's name.

I would like to open another PR after this to rename headerVerifier to blockVerifier. And tapgarden.GenHeaderVerifier to tapgarden.GenBlockVerifier. This rename PR shouldn't hold back the release IMO.

@ffranr ffranr requested review from guggero and jharveyb and removed request for guggero July 17, 2023 19:12
guggero
guggero reviewed Jul 18, 2023
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Looks good! Just one comment about the order of RPC calls, should be a small thing to fix.

chain_bridge.go Outdated Show resolved Hide resolved
chain_bridge.go Outdated Show resolved Hide resolved
chain_bridge.go Show resolved Hide resolved
itest/assertions.go Outdated Show resolved Hide resolved
tapgarden/caretaker.go Outdated Show resolved Hide resolved
proof/proof_test.go Show resolved Hide resolved
jharveyb
jharveyb reviewed Jul 18, 2023
View reviewed changes
Copy link
Collaborator

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

In agreement with previous comments - seems like we should have a negative test in the itests? Or as a unit test, not sure how easy it is to reorg under itest.

@ffranr ffranr force-pushed the add_proof_block_height_verify branch from bad914e to 9532fd7 Compare July 18, 2023 14:03
@ffranr ffranr requested a review from guggero July 18, 2023 14:05
@ffranr ffranr force-pushed the add_proof_block_height_verify branch from 9532fd7 to 389f4c6 Compare July 19, 2023 08:50
@ffranr ffranr requested a review from jharveyb July 19, 2023 08:50
guggero
guggero approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Just two more nits, otherwise LGTM 🎉

chain_bridge.go Outdated Show resolved Hide resolved
proof/proof_test.go Outdated Show resolved Hide resolved
@ffranr ffranr force-pushed the add_proof_block_height_verify branch from 389f4c6 to 03be04b Compare July 19, 2023 09:01
@ffranr
Copy link
Contributor Author

ffranr commented Jul 19, 2023

In agreement with previous comments - seems like we should have a negative test in the itests? Or as a unit test, not sure how easy it is to reorg under itest.

@jharveyb I've updated this PR with an additional negative unit test orientated around block height. Oli mentioned that the re-org work is planned for 0.3.0. Perhaps after that work is complete we can revisit itests for proof block fields?

jharveyb
jharveyb approved these changes Jul 19, 2023
View reviewed changes
Copy link
Collaborator

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Forgot that the reorg work will happen later and separately - LGTM!

@jharveyb jharveyb added this pull request to the merge queue Jul 19, 2023
Merged via the queue into lightninglabs:main with commit 44d1672 Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jharveyb jharveyb jharveyb approved these changes

@guggero guggero guggero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[enhancement] Add block height / GenesisHeight to asset proofs
4 participants
@ffranr @guggero @jharveyb @dstadulis