Page MenuHomePhabricator

[electrum] implement size computation for TxOutput
ClosedPublic

Authored by PiRK on Aug 29 2023, 10:32.

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Aug 29 2023, 10:32
PiRK planned changes to this revision.Aug 29 2023, 13:28

more tests are needed

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

bytesofman added a subscriber: bytesofman.
bytesofman added inline comments.
electrum/electrumabc/tests/test_transaction.py
5 ↗(On Diff #41986)

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 ↗(On Diff #41986)

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 ↗(On Diff #41986)

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
electrum/electrumabc/tests/test_transaction.py
5 ↗(On Diff #41986)

Ok, makes sense. The use of an alias is weird in transaction.py

704 ↗(On Diff #41986)

Good point. But if i remove some test I should probably also limit the sizemethod 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 ↗(On Diff #41986)

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.

PiRK edited the summary of this revision. (Show Details)

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

bytesofman added inline comments.
electrum/electrumabc/tests/test_transaction.py
580 ↗(On Diff #42001)

what's this 37?

This revision now requires changes to proceed.Aug 30 2023, 13:20

fix remaining magic values in test

This revision is now accepted and ready to land.Aug 30 2023, 16:06