Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCdcb410039d3e: [electrum] consolidate compact_size code

Summary

As of D14509, var_int and write_compact_size are doing exactly the same thing. BCDataStream.write_compact_size is also almost doing the same thing. De-triplicate this code.

Use serialize_blob as a shortcut when serializing a push of bytes. Use serialize_sequence as a shortcut when serializing a list of objects that have a serialize method returning bytes. Note that it cannot be done for txinputs because of the extra sign_schnorr argument.

Consolidate also the tests, add tests for the two functions mentioned above, and move compact_size_nbytes to serialize.py

Test Plan

python test_runner.py

grep -r var_int .

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25134
Build 49856: Build Diff	electrum-tests
Build 49855: arc lint + arc unit

Event Timeline

PiRK created this revision.Sep 22 2023, 14:23

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 22 2023, 14:23

PiRK requested review of this revision.Sep 22 2023, 14:23

Harbormaster completed remote builds in B25133: Diff 42344.Sep 22 2023, 14:25

remove linter warning

Harbormaster completed remote builds in B25134: Diff 42345.Sep 22 2023, 14:28

PiRK added inline comments.Sep 22 2023, 14:33

electrum/electrumabc/serialize.py
76–83	I find the `int.to_bytes()` function that was used in `var_int` more readable than `struct.pack` and it is slightly faster too.
97	move-only from transaction.py
electrum/electrumabc/tests/test_serialize.py
12	moved from test_bitcoin.py, the only change is var_int changed to write_compact_size
29	move-only from test_transaction.py
46–81	New tests
electrum/electrumabc_plugins/ledger/ledger.py
517–523	this block was doing a pointless bytes -> hex -> bytes conversion.
electrum/electrumabc_plugins/satochip/satochip.py
431	removed useless log line that avoid a hash computation and allows using `serialize_sequence` as a shortcut

PiRK added a child revision: D14524: [electrum] store and serialize tx data as bytes.Sep 22 2023, 14:39

Fabien requested changes to this revision.Sep 22 2023, 14:50

Fabien added a subscriber: Fabien.

Fabien added inline comments.

electrum/electrumabc/serialize.py
76–83	I agree
97	Is that used in perf critical places ? Otherwise: return len(write_compact_size(varint)) `size` is a bad name for the parameter
electrum/electrumabc/transaction.py
752	Why is that duplicated ?
electrum/electrumabc_plugins/ledger/ledger.py
517–523	oO

This revision now requires changes to proceed.Sep 22 2023, 14:50

PiRK added inline comments.Sep 22 2023, 15:08

electrum/electrumabc/serialize.py
97	I think it may be used in perf critical places. I wrote this function to estimate the transaction size or input size quickly without need to serialize it. The plan is to benefit from this in the CoinChooser to precompute a tx size and avoid selecting too many inputs (wrt to the tx relay size policy)
electrum/electrumabc/transaction.py
752	This method is actually writing the bytes to a buffer. I should rename the `write_compact_size` to just `compact_size`, as the function is not writing anything anywhere, just returning bytes.