bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
832 stars 296 forks source link

electrum: Should `validate_merkle_for_anchor` throw an error for missing or invalid proof? #1508

Open ValuedMammal opened 1 month ago

ValuedMammal commented 1 month ago

Refer to this comment https://github.com/bitcoindevkit/bdk/pull/1489#discussion_r1670822942

_This is a good question and it makes sense to have a runtime option or feature to throw an error if merkle proof validation fails. But I don't think this has to be done in this PR or for the beta release since it will only change the API for the bdkelectrum crate.

ValuedMammal commented 1 month ago

Similarly, should we propagate the error if the transaction_get_merkle API call fails? or are there cases where we expect or allow this to occur, like if given a height of 0?

storopoli commented 1 month ago

~~Yes, it should. It is up to the user to handle the error properly.~~

@evanlinjin has a good point (see below), either we make the Electrum operation atomic or we cannot return an error.

notmandatory commented 1 month ago

I assigned this to the beta milestone since adding the new param and error for this should only affect the bdk_electrum crate, not the bdk_wallet API.

evanlinjin commented 1 month ago

With the electrum API, you can't fetch the header and proof atomically. For both these methods, you fetch header by block height. It's possible that the merkle proof will not match, and not due to a malicious server.

I don't think returning an error makes sense.

tnull commented 1 month ago

With the electrum API, you can't fetch the header and proof atomically. For both these methods, you fetch header by block height. It's possible that the merkle proof will not match, and not due to a malicious server.

You could use this to detect that the reorg happened and that you should restart syncing without applying any changes to avoid inconsistencies. FWIW, this is what we do in LDK's ElectrumSyncClient.