-
Notifications
You must be signed in to change notification settings - Fork 265
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
Use typed TxId #2742
Conversation
535bc34
to
5b8d95b
Compare
Codecov Report
❗ 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
|
There was a problem hiding this 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)
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
Outdated
Show resolved
Hide resolved
8a47a71
to
10e49e7
Compare
Rebased to fix merge conflicts. |
ℹ️ Needs rebase |
And explicitly encode/decode as a `tx_hash` for lightning messages.
10e49e7
to
8917b0c
Compare
Rebased. |
There was a problem hiding this 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.
And explicitly encode/decode as a
tx_hash
for lightning messages. This mostly a trivial replacement ofByteVector32
byTxId
with the removal of several calls to.reverse
, but the subtle parts that should be thoroughly reviewed are:BitcoinCoreClient.scala
-> some bitcoind RPCs callhash
something that is actually encoded as anid
, which is very confusingwire
package is where we actually encode as atx_hash
orblock_hash
to be spec-compliant'
andh
for BIP 32 hardened indices inLocalOnChainKeyManager.scala
now thatbitcoin-lib
supports both