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

Check tx confirmation proofs #2373

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Check tx confirmation proofs #2373

wants to merge 3 commits into from

Conversation

sstone
Copy link
Member

@sstone sstone commented Aug 10, 2022

When a tx is confirmed. ask for and check a tx inclusion proof, and check the p.o.w of the block in which it was published as well as the following blocks.

When we're notified that a tx has been confirmed, we:
- ask bitcoind for a "txout" proof i.e a proof that the tx was included in a block
- verify this proof
- verify the proof of work of the block in which it was published and its descendants by checking that
the block hash matches the block difficulty and (only on mainnet) that the diffculty is above a given target
Copy link
Contributor

@remyers remyers left a comment

Choose a reason for hiding this comment

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

This looks good! I found only one concrete problem where a malicious bitcoind could trick us into not checking the proof.
edit: Only have a few questions and nits.

@codecov-commenter
Copy link

Codecov Report

Merging #2373 (28dcda2) into master (fa985da) will decrease coverage by 0.02%.
The diff coverage is 88.88%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #2373      +/-   ##
==========================================
- Coverage   85.71%   85.69%   -0.02%     
==========================================
  Files         214      214              
  Lines       17558    17579      +21     
  Branches      754      754              
==========================================
+ Hits        15050    15065      +15     
- Misses       2508     2514       +6     
Impacted Files Coverage Δ
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala 86.46% <80.00%> (-0.84%) ⬇️
...ir/blockchain/bitcoind/rpc/BitcoinCoreClient.scala 98.66% <100.00%> (+0.22%) ⬆️

... and 4 files with indirect coverage changes

We can use the tx position that we compute when we verify the inclusion proof instead of explicitly asking Bitcoin Core for it.
We also check that the id of the tx that is returned match what we asked for, and add min-difficulty checks for regtest and testnet.
All blocks now are BIP34 compliant commit to their height in the sig script
of their coinbase tx.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants