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

Use typed TxId #2742

Merged
merged 3 commits into from
Nov 23, 2023
Merged

Use typed TxId #2742

merged 3 commits into from
Nov 23, 2023

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Sep 13, 2023

And explicitly encode/decode as a tx_hash for lightning messages. This mostly a trivial replacement of ByteVector32 by TxId with the removal of several calls to .reverse, but the subtle parts that should be thoroughly reviewed are:

  • BitcoinCoreClient.scala -> some bitcoind RPCs call hash something that is actually encoded as an id, which is very confusing
  • the wire package is where we actually encode as a tx_hash or block_hash to be spec-compliant
  • I cleaned up the confusion between ' and h for BIP 32 hardened indices in LocalOnChainKeyManager.scala now that bitcoin-lib supports both

@t-bast t-bast marked this pull request as ready for review October 4, 2023 15:13
@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

Merging #2742 (8917b0c) into master (0a833a5) will increase coverage by 0.00%.
Report is 3 commits behind head on master.
The diff coverage is 92.46%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2742   +/-   ##
=======================================
  Coverage   85.85%   85.86%           
=======================================
  Files         216      216           
  Lines       18132    18169   +37     
  Branches      775      832   +57     
=======================================
+ Hits        15568    15600   +32     
- Misses       2564     2569    +5     
Files Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 55.69% <100.00%> (ø)
...core/src/main/scala/fr/acinq/eclair/Features.scala 100.00% <100.00%> (ø)
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 75.14% <ø> (ø)
...ala/fr/acinq/eclair/blockchain/OnChainWallet.scala 100.00% <100.00%> (ø)
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala 86.61% <100.00%> (ø)
...chain/bitcoind/rpc/BasicBitcoinJsonRPCClient.scala 100.00% <100.00%> (ø)
...ir/blockchain/bitcoind/rpc/BitcoinCoreClient.scala 88.23% <100.00%> (ø)
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala 95.34% <100.00%> (ø)
...lair/blockchain/watchdogs/BlockchainWatchdog.scala 61.53% <100.00%> (ø)
...cinq/eclair/blockchain/watchdogs/ExplorerApi.scala 45.09% <ø> (ø)
... and 61 more

... and 4 files with indirect coverage changes

Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits: NewBlock and ProcessNewBlock should use a BlockId and not a ByteVector32 (which is another instance where bitcoin core's API is mixing block/tx ids and hashes)

sstone
sstone previously approved these changes Oct 31, 2023
@t-bast
Copy link
Member Author

t-bast commented Nov 10, 2023

Rebased to fix merge conflicts.

sstone
sstone previously approved these changes Nov 13, 2023
@pm47
Copy link
Member

pm47 commented Nov 23, 2023

ℹ️ Needs rebase

And explicitly encode/decode as a `tx_hash` for lightning messages.
@t-bast
Copy link
Member Author

t-bast commented Nov 23, 2023

Rebased.

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

A much needed change! Finally getting rid of the ambiguity.

I would have used TxHash, not TxId in LN protocol messages, but it's just a preference.

There seem to be a few BlockId/BlockHash inversions.

@t-bast t-bast merged commit e73c1cf into master Nov 23, 2023
1 check passed
@t-bast t-bast deleted the use-txid-txhash branch November 23, 2023 15:04
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.

4 participants