T3225
This diff updates getInputUtxos to take in additional params for custom output count and OP_RETURN bytecount. This is to prepare this library for subsequent SLP supporting diffs.
Details
Details
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- OpReturnSupport
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 24946 Build 49480: Build Diff ecash-coinselect Build 49479: arc lint + arc unit
Event Timeline
| modules/ecash-coinselect/src/utxo.js | ||
|---|---|---|
| 30 ↗ | (On Diff #41963) | Look carefully at the code and you will find out that there is something missing in this API. |
Comment Actions
- Renamed the existing getInputUtxos() function into getP2pkhInputUtxos(), along with README and commenting updates to make it clear this function ONLY takes in p2pkh utxos. Similarly updated mock names to reflect this.
- Added new 'getInputUtxos() function that is similar to getP2pkhInputUtxos() except it takes in an eCash address, converts it to hash160 and makes the chronik.script(type, hash).utxos()` call directly before parsing the result.
- Due to the similarity between getP2pkhInputUtxos() and 'getInputUtxos(), the common logic has been moved into a new function called collectXecUtxos()`.
- @TODO: need to add ChronikMock in a separate diff in order to mock the chronik.script().utxos() api call within getInputUtxos().
Comment Actions
@TODO: need to add ChronikMock in a separate diff in order to mock the chronik.script().utxos() api call within getInputUtxos().
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (ecash-coinselect) failed.
Tail of the build log:
Run `npm audit` for details.
> ecash-coinselect@1.0.2 test
> mocha --reporter mocha-junit-reporter --reporter-options mochaFile=test_results/ecash-coinselect-junit.xml --reporter-options testsuitesTitle=Ecash Coinselect Unit Tests --reporter-options rootSuiteTitle=Ecash Coinselect
Error: Cannot find module 'ecashaddrjs'
Require stack:
- /work/modules/ecash-coinselect/src/utxo.js
- /work/modules/ecash-coinselect/test/utxo.test.js
at Function.Module._resolveFilename (node:internal/modules/cjs/loader:1028:15)
at Function.Module._load (node:internal/modules/cjs/loader:873:27)
at Module.require (node:internal/modules/cjs/loader:1100:19)
at require (node:internal/modules/cjs/helpers:119:18)
at Object.<anonymous> (/work/modules/ecash-coinselect/src/utxo.js:5:133)
at Module._compile (node:internal/modules/cjs/loader:1198:14)
at Module.replacementCompile (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:60:13)
at Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
at Module.load (node:internal/modules/cjs/loader:1076:32)
at Function.Module._load (node:internal/modules/cjs/loader:911:12)
at Module.require (node:internal/modules/cjs/loader:1100:19)
at require (node:internal/modules/cjs/helpers:119:18)
at Object.<anonymous> (/work/modules/ecash-coinselect/test/utxo.test.js:11:5)
at Module._compile (node:internal/modules/cjs/loader:1198:14)
at Module.replacementCompile (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:60:13)
at Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
at Module.load (node:internal/modules/cjs/loader:1076:32)
at Function.Module._load (node:internal/modules/cjs/loader:911:12)
at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:169:29)
at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
at async Promise.all (index 0)
at async ESMLoader.import (node:internal/modules/esm/loader:530:24)
at async importModuleDynamicallyWrapper (node:internal/vm/module:438:15)
at async formattedImport (/work/modules/ecash-coinselect/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
at async Object.exports.requireOrImport (/work/modules/ecash-coinselect/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
at async Object.exports.loadFilesAsync (/work/modules/ecash-coinselect/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
at async singleRun (/work/modules/ecash-coinselect/node_modules/mocha/lib/cli/run-helpers.js:125:3)
at async Object.exports.handler (/work/modules/ecash-coinselect/node_modules/mocha/lib/cli/run.js:370:5)
----------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files | 1.47 | 0 | 0 | 1.51 |
utxo.js | 1.47 | 0 | 0 | 1.51 | 6-326
----------|---------|----------|---------|---------|-------------------
##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='1']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='68']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='0']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='22']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='0']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='5']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='1']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='66']
##teamcity[blockClosed name='Code Coverage Summary']
mv: cannot stat 'test_results/ecash-coinselect-junit.xml': No such file or directory
Build ecash-coinselect failed with exit code 1Comment Actions
Rebased to D14423 and used the mock chronik from there to add new unit tests which mock the script().utxos() API calls from within getInputUtxos.
Comment Actions
This API doesn't work, on one hand you have the output count and on the other hand you expect the output to be a certain kind + have some op_return bytes.
Are the op_return output part of the output_count ? What if I have 250 p2pkh outputs and 10 op_return outputs ? The varint won't be updated.
| modules/ecash-coinselect/src/utxo.js | ||
|---|---|---|
| 32 | I'm still not 100% sure if this should be public. I can see the value for an app that will do its own call to chronik as it avoids duplicating the call, but that seems to be an edge case | |
| 38 | Is this really needed as an input ? Why would the caller change that ? | |
Comment Actions
Responding to feedback
Are the op_return output part of the output_count ?
No, the op_return outputs don't form part of the output count on the basis that op_return output is designated as unspendable.
What if I have 250 p2pkh outputs and 10 op_return outputs ?
The initial use case for this diff is a standard SLP tx, which only has one op_return output (index 0) along with p2pkh outputs as dust.
As per SLP spec below, any other op_return outputs after index 0 is ignored by SLP.
| modules/ecash-coinselect/src/utxo.js | ||
|---|---|---|
| 32 | Would you by any chance be referring to getInputUtxos instead? As that's the function that makes the chronik call. getP2pkhInputUtxos just takes in a utxo set. | |
| 38 | The original use case for this diff was to cater for SLP genesis, send and burn transactions. SLP genesis txs only needs two outputs:
SLP send and burn txs would need four outputs:
Hence the need to override this outputCount based on the token action as per below:  | |
Comment Actions
I think you're confused here.
The output count is the actual txout vector of the transaction. It doesn't matter whether this is spendable or not, an op_return is unspendable but it's still an output of the transaction, and it adds 1 to the length of the txout vector.
Having the output count be an inout parameter, and even worse with a default value (an out parameter with a default value makes absolutely zero sense) is also very convoluted. Like if I say I want 2 outputs, you can generate a single one because there is no change ? Is that the expected behavior ? Why should the callsite have to deal with a change output when the library is the one selecting the coins ?
You should really think the API first before you start coding this. And not only the single use case of your example, but what a random eCash dev is expecting from a library called ecash-coinselect.
You can also look at what the other libs are doing. There even is (at least one, likely more) an online tx size calculator the has a form which is mostly what your API should look like.
Comment Actions
think the API first before you start coding this
Point taken re: the outputCount.
I had a look around, most notably the coinselect API consists of taking in:
- utxo set
- targets (i.e. the intended outputs and their values)
- fee rate
So reviewing the current ecash-coinselect API:
- I would drop the outputCount param and instead pass in a tokenAction param.
- Based on this tokenAction, collectXecUtxos will then determine whether it's two outputs for an SLP Genesis or four outputs for SLP SEND/BURN txs. If tokenAction is null then it would equate to the standard one to one XEC tx with two outputs.
- It no longer need to cater for custom output counts. Subsequent diffs can deal with this for one to many txs...etc where sendAmountInSats becomes an object array of multiple targets.
- The single non-spendable SLP op_return output is to be incorporated into the output size of the fee calcuation logic.
- The callsite executing getP2pkhInputUtxos will no longer need to deal with output counts.
i.e.
function getP2pkhInputUtxos(
chronikP2pkhUtxos,
sendAmountInSats,
tokenAction,
opReturnByteCount = 0,
)
...
return collectXecUtxos(
chronikUtxos,
sendAmountInSats,
tokenAction,
opReturnByteCount,
);Is this approach feasible?
Comment Actions
The token action seems like not what you want, at least not from the beginning.
You can look at https://jlopp.github.io/bitcoin-transaction-size-calculator/ for inspiration. This can be extended with op_return outputs as needed.
You should do it incrementally:
- Get a working API for N x p2pkh outputs
- Extend with N x p2sh outputs
- Extend with N x op_return
Now it's trivial to wrap up this selection function to make it easier to deal with the most common usages.