Upgrade to bitcoin-kmp 0.31.0#108
Conversation
Wrapping a bitcoin-kmp transaction instance is much more efficient and benefits from optimizations added in ACINQ/bitcoin-kmp#184, at the cost of having to provide "update" methods instead of using `.copy()`. We also use `.toArrayUnsafe()` to convert scodec byte vectors to byte arrays, which should be more efficient (we use mostly small byte arrays, which would be simply wrapped by scodec and would be passed directly to bitcoin-kmp instead of creating copies).
sstone
force-pushed
the
update-bitcoin-kmp
branch
from
f417361 to
5b9d9b2
Compare
| /** | ||
| * Transaction | ||
| * Instead of using a pure Scala case class, we simply wrap an instance of bitcoin-kmp transaction. | ||
| * This is done because kmp<->scala conversion is expensive and to benefit from optmisations (such as |
There was a problem hiding this comment.
nit:
| * This is done because kmp<->scala conversion is expensive and to benefit from optmisations (such as | |
| * This is done because kmp<->scala conversion is expensive and to benefit from optimisations (such as |
| * @param lockTime The block number or timestamp at which this transaction is locked | ||
| */ | ||
| case class Transaction(version: Long, txIn: Seq[TxIn], txOut: Seq[TxOut], lockTime: Long) extends BtcSerializable[Transaction] { | ||
| case class Transaction(inner: bitcoinkmp.Transaction) extends BtcSerializable[Transaction] { |
There was a problem hiding this comment.
We need to test that change on eclair before deciding whether we want to move forward with this. I honestly don't know if this provides meaningful benefits compared to the additional complexity: we don't validate blocks in eclair, and we don't frequently broadcast transaction (only when funding, splicing and closing, which isn't on the hot path for performance).
The only place where we do transaction signing a lot is when sending/receiving commit_sig, but is that worth it? I think we need to do some benchmarking of the current code and the updated code with that change to see if it has a real impact or not. We must make sure this is worth the extra complexity.
There was a problem hiding this comment.
I think it's worth it (it also speeds up serialisation and would enable further optimisations in bitcoin-kmp) but should be done in another PR. I've reverted Transaction back to a standard case class, and added a lazy val to store its bitcoin-kmp counterpart and avoid re-computing it.
This will have less impact downstream and sill benefits from bitcoin-kmp optimisations.
sstone
force-pushed
the
update-bitcoin-kmp
branch
from
6460eb7 to
defabf9
Compare
t-bast
left a comment
t-bast
left a comment
There was a problem hiding this comment.
I have no idea what the pom.xml changes do, but apart from that everything look good.
They fix the snapshot configuration so we can easily publish snapshots and remove a redondant gpg configuration section. |
2 participants
We update bitcoin-lib to 0.31.0 which includes some API changes (see https://github.com/ACINQ/bitcoin-kmp/releases/tag/v0.31.0).