[ecash-coinselect] Update getInputUtxos to support OP_RETURN scripts in outputArray
AbandonedPublic
Actions

Authored by emack on Sep 15 2023, 04:21.

Details

Reviewers

Fabien
bytesofman

Group Reviewers

Restricted Project

Summary

T3225

As per feedback from D14417, this diff forms part of a series of diffs to incrementally implement the key concepts from D14417:

The coinselection and txFee logic takes into consideration both the extra OP_RETURN outputs and the byte size of each OP_RETURN script.

1. Get a working API for N x p2pkh outputs
2. Extend with N x op_return **(this diff)**
3. Extend with N x slp outputs
4. Extend with N x p2sh outputs

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Branch

opreturnOutputs

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25049
Build 49686: Build Diff	ecash-coinselect
Build 49685: arc lint + arc unit

Event Timeline

emack created this revision.Sep 15 2023, 04:21

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 15 2023, 04:21

emack requested review of this revision.Sep 15 2023, 04:21

emack edited the summary of this revision. (Show Details)Sep 15 2023, 04:23

Harbormaster completed remote builds in B25045: Diff 42218.Sep 15 2023, 04:23

emack edited the summary of this revision. (Show Details)Sep 15 2023, 04:32

We have a problem here.

The diff has a bug, but that's not the big deal. The problem is that the tests pass, which is VERY concerning. This means that the tests cannot be trusted, and potentially that all the previous work is wrong. How did you build your tests, and especially how did you compute the expected txFee?

modules/ecash-coinselect/README.md
81 ↗	(On Diff #42218)	What is the reason for using a Buffer here ? IMO we should not enforce the callsite to add an external dependency to use our library. What about passing the hex string, and the doing the buffer conversion internally if that is required ? Afaict Buffer is a node feature. Are we expecting all calling apps to use node ?
modules/ecash-coinselect/src/utxo.js
23 ↗	(On Diff #42218)	Nit: indentation is broken
77 ↗	(On Diff #42218)	Now this is wrong

This revision now requires changes to proceed.Sep 15 2023, 07:46

Responding to feedback

How did you build your tests, and especially how did you compute the expected txFee?

Since the txFee is (bytecount + opReturnScriptBytes) * 1 sat per byte, I would normally add in a few debug statements to print out the total bytecounts (previously unit tested), which evaluates to be the txFee in sats.

From there:
Change = Total input utxo value - total send value - txFee

i.e. taking the getInputUtxos() correctly collects enough XEC utxos for an outputArray containing an OP_RETURN output test as an example:

Total input utxo value: 38952
Total send value: 14900
Bytecount: 476
opReturnScriptBytes: 11 (22 char hex string / 2)
Final txFee: (Bytecount + opReturnScriptBytes) * 1 sat per byte = (476+11)*1 = 487
Change: 38952 - 14900 - 487 = 23565

For the getInputUtxos() correctly collects enough XEC utxos for an outputArray containing multiple OP_RETURN outputs test:

Total input utxo value: 38952
Total send value: 14900
Bytecount: 510
opReturnScriptBytes: 179 (22/2 + 336/2)
Final txFee: (Bytecount + opReturnScriptBytes) * 1 sat per byte = (510+179)*1 = 689
Change: 38952 - 14900 - 689 = 23363

However, since you've noted the let byteCount = calcP2pkhByteCount(inputUtxos.length, outputCount); line is incorrect for this opreturn context, then that would be the cause of the unit tests incorrectly passing in this diff.

modules/ecash-coinselect/README.md
81 ↗	(On Diff #42218)	Removed use of Buffer and passing hex string instead
modules/ecash-coinselect/src/utxo.js
23 ↗	(On Diff #42218)	Fixed indentation
77 ↗	(On Diff #42218)	Hmm... the `outputCount` is already inclusive of the op_return output. Are you expecting the op_return script byte size to be included in this call? Or should this be a separate calcOpReturnTxByteCount() function?

Harbormaster completed remote builds in B25049: Diff 42223.Sep 15 2023, 13:25

bytesofman requested changes to this revision.Sep 15 2023, 13:30

bytesofman added inline comments.

modules/ecash-coinselect/src/utxo.js
77 ↗	(On Diff #42218)	At least, this is no longer `byteCount`, it's more like `byteCountLessOpReturnBytes` ... and kind of confusing that it comes from `calcP2pkhByteCount` function, since now the answer is not `p2pkhByteCount` but only a portion of it. This is "corrected" below by calculating txfee on `byteCount + opReturnScriptBytes` ... i.e., on the real `byteCount`. Confusing model where `getInputUtxos` calculates the total byte count and `getP2pkhByteCount` calculates something else.
100 ↗	(On Diff #42218)	also requires this manual addition in two places for it to work
modules/ecash-coinselect/test/utxo.test.js
326–327 ↗	(On Diff #42218)	slp-mdm library will be obsolete with upcoming versions of slp. We don't want to make ecash-coinselect tied to specific dependencies. `string` is more versatile.

This revision now requires changes to proceed.Sep 15 2023, 13:30

Fabien added inline comments.Sep 15 2023, 13:49

modules/ecash-coinselect/src/utxo.js
77 ↗	(On Diff #42218)	Unless I'm blind, the op_return IS included in the outputCount. So this is accounting for N p2pkh, N being num(p2pkh) + num(op_return).

emack marked 2 inline comments as done.Sep 15 2023, 14:01

emack added inline comments.

modules/ecash-coinselect/src/utxo.js
77 ↗	(On Diff #42218)	Does that mean because I've included the OP_RETURN in the output count, I shouldn't be adding the OP_RETURN script bytesize on top? In `calcP2pkhByteCount` the output sizes are 34 bytes but OP_RETURN scripts can be much larger than that, hence its addition.
modules/ecash-coinselect/test/utxo.test.js
326–327 ↗	(On Diff #42218)	Yup removed