Add get_block_headers and check timestamps
#669
Conversation
wszdexdrf
changed the title
get_block_headers and check timestamps
It adds a new function get_block_headers to the original get_block_hash()
wszdexdrf
force-pushed
the
get_block_headers
branch
from
4d2b4b0 to
5541f1d
Compare
There was a problem hiding this comment.
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?
|
Sorry, but I'm a bit lost... You're saying that the functions in |
danielabrozzoni
modified the milestones:
Release 0.23.0 Feature Freeze,
Release 0.24.0 Feature Freeze
|
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. |
Description
This changes
GetBlockHashtoGetBlockInfo. A new function is added to the new trait (in addition to the old oneget_block_hash) which isget_block_headers, which returns block headers for a given block height. It is implemented on all blockchain backends.This also changes
AfterandOlderdefined inwallet/utils.rsto accept and check against timestamps.Notes to the reviewers
This PR was meant to fix bitcoindevkit/bdk_wallet#183, but further testing revealed that (a) the checking functions in
wallet/utils.rsaren't actually called, the implementations inminiscript/satisfy.rsare called, which already check timestamps; and (b) the checking functions inwallet/utils.rshave 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 thewallet/utils.rsfunctions, without changing the current flow of code.Hence the issue is already fixed and this PR barely changes anything.
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md