Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Commits

rABC0895d058526b: [electrum] implement size computation for TxOutput

Summary

This will be useful for making the coinchooser not produce transactions that are larger than the 100kB relay policy.

Depends on D14431

Test Plan

python test_runner.py

Diff Detail

Repository

rABC Bitcoin ABC

Branch

coinchooser_find_size

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 24886
Build 49360: Build Diff	electrum-tests
Build 49359: arc lint + arc unit

Event Timeline

PiRK created this revision.Aug 29 2023, 10:32

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 29 2023, 10:32

PiRK requested review of this revision.Aug 29 2023, 10:32

Harbormaster completed remote builds in B24881: Diff 41981.Aug 29 2023, 10:34

more tests are needed

simplify code, test boundaries for script size wrt compact size encoding

Harbormaster completed remote builds in B24886: Diff 41986.Aug 29 2023, 13:50

bytesofman requested changes to this revision.Aug 29 2023, 15:19

bytesofman added a subscriber: bytesofman.

bytesofman added inline comments.

electrum/electrumabc/tests/test_transaction.py
5	in `transactions.py`, `OpCodes` is imported as `opcodes`, and here it is not -- any reason for this discrepancy? It's not clear to me why `opcodes` alias is used in `transactions.py` but seems like it should have the same value in both files unless there is some syntax reason going on
704	do we need these last two then? seems like a large memory req. I'm not familiar with what kind of practical limits we should be setting for the demands of monorepo CI unit tests.
electrum/electrumabc/transaction.py
100–104	I'm not sure what the magic numbers are for here 253 +1 +3 0x10000 0x100000000 +5 +9

This revision now requires changes to proceed.Aug 29 2023, 15:19

PiRK added inline comments.Aug 29 2023, 18:01

electrum/electrumabc/tests/test_transaction.py
5	Ok, makes sense. The use of an alias is weird in transaction.py
704	Good point. But if i remove some test I should probably also limit the `size`method to refuse to handle these cases. I doubt that we will ever see a 4GB locking script on chain, but i'm not sure what the actual limit is.
electrum/electrumabc/transaction.py
100–104	OK. I'm going to encapsulate all of this in a `compact_size_nbytes` function with a comment and/or a link to the compact size spec, to make it clearer what is going on. I will need to reuse the same code when I add a TxInput class anyway, so the function, will be used in more than just this place.

Add a compact_size_nbytes function with link to the spec. Fix test completeness: limit script size to MAX_SCRIPT_SIZE, test the new function on all possible boundary values

PiRK added a child revision: D14432: [electrum] move serialize.py out of avalanche.Aug 30 2023, 08:05

Harbormaster completed remote builds in B24896: Diff 42001.Aug 30 2023, 08:24

bytesofman requested changes to this revision.Aug 30 2023, 13:20

bytesofman added inline comments.