bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Add `get_block_headers` and check timestamps

Open wszdexdrf opened this issue 3 years ago • 2 comments

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:

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

New Features:

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

wszdexdrf avatar Jul 15 '22 07:07 wszdexdrf

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?

danielabrozzoni avatar Sep 26 '22 12:09 danielabrozzoni

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.

wszdexdrf avatar Sep 26 '22 12:09 wszdexdrf