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

Add get_block_headers and check timestamps #669

Closed
wants to merge 2 commits into from

Conversation

wszdexdrf
Copy link
Contributor

@wszdexdrf wszdexdrf commented Jul 15, 2022

Description

This changes GetBlockHash to GetBlockInfo. A new function is added to the new trait (in addition to the old one get_block_hash) which is get_block_headers, which returns block headers for a given block height. It is implemented on all blockchain backends.

This also changes After and Older defined in wallet/utils.rs to accept and check against timestamps.

Notes to the reviewers

This PR was meant to fix #642, but further testing revealed that (a) the checking functions in wallet/utils.rs aren't actually called, the implementations in miniscript/satisfy.rs are called, which already check timestamps; and (b) the checking functions in wallet/utils.rs have a slightly different purpose (checking if the transaction can be broadcasted) compared to miniscript implementations (checking if the transaction timelocks are satisfied). Hence I changed the wallet/utils.rs functions, without changing the current flow of code.
Hence the issue is already fixed and this PR barely changes anything.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@wszdexdrf wszdexdrf changed the title Get block headers Add get_block_headers and check timestamps Jul 15, 2022
@notmandatory notmandatory added the new feature New feature or request label Jul 16, 2022
It adds a new function get_block_headers to the original
get_block_hash()
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Concept ACK on f2634fc..

I will go through the 2nd commit more thoroughly, but I really think that should have been a separate PR..

So if you are still working on it, my initial thought is, why not just remove the GetBlockHash trait all together?? A hash from a header is just a .block_hash() call away on the Header.. So the Header is really what we want most of the time, for time, hash whatever..

So if you are changing the trait anyway, why not just simply have a GetHeader trait?

@danielabrozzoni
Copy link
Member

Sorry, but I'm a bit lost... You're saying that the functions in wallet/utils.rs aren't actually called, then why is this PR modifying them? Can't we just wipe them and use the miniscript ones?

@wszdexdrf
Copy link
Contributor Author

Maybe I should explain. The reason I changed the functions (which were not actually used) was because I was unsure of which way to solve the issue. On one hand, the miniscript's function solve the problem, but they don't actually solve it strictly correctly (using median time past etc.). Therefore I changed the functions (in hopes that maybe we could change the calls in order to call our implementation (somehow) which would check more thoroughly (using block times and what-not). Though I didn't change the calls, I changed the functions in order to show how exactly the code will look, before changing how it works. Hence the reason this was a draft PR.

@notmandatory notmandatory removed this from the Release 0.24.0 Feature Freeze milestone Oct 11, 2022
@danielabrozzoni danielabrozzoni mentioned this pull request Oct 27, 2022
8 tasks
@wszdexdrf wszdexdrf closed this Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

timestamp timelocks are not satisfied
4 participants