Page MenuHomePhabricator

[Cashtab] Implement ecash-lib for tx building
ClosedPublic

Authored by bytesofman on May 21 2024, 13:07.

Details

Reviewers
Fabien
tobias_ruck
Group Reviewers
Restricted Project
Commits
rABCebfc56b49c6b: [Cashtab] Implement ecash-lib for tx building
Summary

T3582

Implement ecash-lib for building txs in Cashtab.

This diff uses ecash-lib instead of utxo-lib + ecash-coinselect to build raw txs with schnorr signatures (previously all Cashtab txs were ECDSA).

Because ecash-coinselect is calibrated for ECDSA, it does not provide accurate fee estimation for Schnorr txs (hence, it also does not provide accurate estimation of the maximum amount of satoshis a wallet can send, or a useful utxo selection algorithm).

The key feature of ecash-coinselect --- determining whether or not a tx needs a change output, and what the value of that output should be given the desired tx fee --- is handled by ecash-lib. Going forward, we may wish to add utxo selection algorithms to ecash-lib. For now, since Cashtab only uses a simple accumulative algorithm and this was all that ecash-coinselect provided -- simply deprecate ecash-coinselect.

Since ecash-lib already does most of what ecash-coinselect used to do (but better and for schnorr), ecash-coinselect should go through a deprecation cycle. No point in adding new features to the lib.

There is still more work to be done to fully implement ecash-lib in Cashtab. ecash-lib covers features where Cashtab is currently still using utxo-lib or slp-mdm. These will be deprecated in subseqent diffs (see T3582).

Test Plan

npm test

Select 2 or 3 of the changed txs from the tests at random. Import the old tx into Electrum and compare to the new tx.

Confirm the inputs and outputs are the same. Differences should be

  • txid is different
  • size is different
  • fee is lower in absolute terms

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

specify chaintipBlockheight at callsite of getMaxSendAmountSatoshis

do not build ecash-coinselect for cashtab tests in CI

tobias_ruck added a subscriber: tobias_ruck.
tobias_ruck added inline comments.
cashtab/src/transactions/index.js
258 ↗(On Diff #47892)

this way the user doesn't have to specify ecc, and we don't have to create signatures that are thrown away again

This revision now requires changes to proceed.May 22 2024, 17:55
cashtab/src/transactions/index.js
186 ↗(On Diff #47898)

broadcastTx decodes the hex again anyway

bytesofman added inline comments.
cashtab/src/transactions/index.js
258 ↗(On Diff #47892)

if I use the EccDummy approach (and therefore no ecc) --- how should the inputs be refactored so that the size is still correctly considered?

ref line 219, above

cashtab/src/transactions/index.js
186 ↗(On Diff #47898)

dang...so, this would be better.

However, all of the chronik mocks are set with raw hex, which is much easier to manage in the tests than UINT8 arrays (raw hex is easy to copy paste into electrum abc to check, easy to console.log when testing new txs).

Since Cashtab isn't really machine gunning txs I think this isn't worth changing.

cashtab/src/transactions/index.js
42 ↗(On Diff #47898)

shouldn't this be based on minFee from the settings?
also, any reason to check this here?

186 ↗(On Diff #47898)

makes sense, it's cheap enough anyway so that should be fine

224 ↗(On Diff #47898)

to estimate the tx size, you don't need the actual private key here, you can just set a dummy, they will result in the same estimation.

225 ↗(On Diff #47898)

if you use the EccDummy here, the derive will be free

254 ↗(On Diff #47898)

Same here, you can just put a dummy P2PKH script

bytesofman added inline comments.
cashtab/src/transactions/index.js
42 ↗(On Diff #47898)

Probably not really a worthwhile check, esp in this function which can only be called using a fee param that is only dev-set. I added this in this diff to preserve the behavior of a unit test from before this migration, which was throwing an error from ecash-coinselect.

removed.

bytesofman marked an inline comment as done.

remove min fee check, use dummy ecc for calculating max send amount

cashtab/src/transactions/fixtures/vectors.js
281 ↗(On Diff #47898)

this unit test was confirming expected behavior of ecash-coinselect, so kind of "extra" even before this implementation.

In general, we do not need to be running a min fee check every time a tx is built (when the min fee param cannot be set by the user).

Fabien requested changes to this revision.May 24 2024, 15:24
Fabien added inline comments.
cashtab/src/components/Configure/__tests__/Configure.test.js
171 ↗(On Diff #47903)

I still don't understand why you have to change this. Mind explaining ?

This revision now requires changes to proceed.May 24 2024, 15:24
bytesofman added inline comments.
cashtab/src/components/Configure/__tests__/Configure.test.js
171 ↗(On Diff #47903)

Cashtab is now creating raw txs that are schnorr signed instead of ECDSA signed. So, the raw hex is different than before.

the way chronik-client works, rawtxs are broadcast with chronik.broadcastTx(rawTx)

if the tx is successfully broadcast, then chronik-client returns the txid.

mock-chronik-client is dumb. you just tell it what call to return for an expected call and it returns it. In these tests, we are telling mock-chronik-client "you should expect a chronik.broadcastTx call for this particular hex string, and, when you receive it, please return this txid, to mock a successfully broadcast tx in prod."

If we do not update the rawtx in the mock

  • we set mock-chronik-client with ECDSA_rawtx
  • cashtab builds a rawtx with schnorr, and schnorr_rawtx !== ECDSA_rawtx
  • the function tries to broadcast schnorr_rawtx but the mock has not been set for this, so it returns a 'not found' error

So, the unit test(s) and integration tests in Cashtab are really testing that Cashtab is building the expected rawtx. We don't really test that chronik successfully broadcasts the txs. But, we need this mock bc Cashtab is set up to actually broadcast txs, which are different every time.

In the integration test we can then confirm that we show a notification correctly (since we have set the txid response).

This diff changes every tx that Cashtab builds from ECDSA to schnorr, so all of the mocks need to be changed.

bytesofman added inline comments.
cashtab/src/components/Configure/__tests__/Configure.test.js
157 ↗(On Diff #47903)

here's the tx Cashtab built using utxo-lib

image.png (709×798 px, 55 KB)

171 ↗(On Diff #47903)

Here's the tx Cashtab is building now, using ecash-lib

image.png (709×798 px, 57 KB)

tobias_ruck added inline comments.
cashtab/src/transactions/index.js
229 ↗(On Diff #47903)

btw, since we don't sign anything, you could also just build a Tx directly, by simply having a dummy P2PKH input repeated N times, without having to set any signData or a signatory and simplify this quite a bit.

but up to you, if cashtab in the future has more complex input types (e.g. multisig) the current structure is better

265 ↗(On Diff #47903)
bytesofman marked 2 inline comments as done.

eccDummy instead of new EccDummy() (already called)

cashtab/src/transactions/index.js
229 ↗(On Diff #47903)

good point. for now will leave it as is

  • have to modify the outputs conditionally already (may need to do this with inputs at some point in the future)
  • not a function that is called frequently (can only be called by manual user action)
  • functional / readable
tobias_ruck added inline comments.
cashtab/src/transactions/index.js
128 ↗(On Diff #47962)

It seems weird to mix both pre-selected UTXOs for token UTXOs, and then have a selection algorithm for sats. IMO either have both preselected (presumably in one array), or select both in this function.

182 ↗(On Diff #47962)

There's other errors that may be thrown here (e.g. missing SignData), so IMO you should check if the error starts with "Insufficient input value", and let it bubble up otherwise.

The general approach seems weird though. IMO you should do what I recommended previously:

In your case, since P2PKH inputs have a fixed size, you can just estimate the size once using a tx without any fuel inputs + a 0 value change output and then add UTXOs, where each has a value of utxo.value - P2PKH input size.

Then, you just select the needed UTXOs (accumulatively), probably first the token UTXOs, and then fill up the missing sats (based on requiredSats = (txSize * satsPerKb + 999) / 1000 + sum(outputs.value))

Later, we can perhaps factor this out into ecash-lib.

211 ↗(On Diff #47962)

isn't this an array? shouldn't it be scriptOutputs then?

This revision now requires changes to proceed.May 26 2024, 20:39
bytesofman added inline comments.
cashtab/src/transactions/index.js
128 ↗(On Diff #47962)

It is a bit strange. I think it's the best way, at least for now.

  • All txs are XEC txs, so all txs will need to determine eCash utxos
  • Token txs all have different rules. Is this SLP1 or 2? NFT? genesis? Send? burn? ALP? If NFT, is this a fan-out tx? etc.

We handle the token input selection in a variety of functions that do not play nicely with each other as they all have different rules. See slpv1/index.js

There is probably an opportunity to clean it up a bit by calling something like getTokenInputs(tokenType, txType, options) in this function. However, I think this should be implemented in its own diff after Cashtab is fully on schnorr.

I have also for example left out slp-mdm deprecation in this diff -- ecash-lib handles token OP_RETURN creation better, but there are so many call sites for this that imo it would be overloading this diff to handle it here.

Getting this diff in gives us a baseline of expected schnorr-signed rawtxs. Then, when we swap out slp-mdm for ecash-lib, we can check the implementation is good as we should then expect the same rawTxs.

182 ↗(On Diff #47962)

imo this introduces too much complexity. The sign function in ecash-lib is already making estimates with a dummy tx...so now we make an estimate before the estimate?

What if we estimate here assuming a change output, think we need another output to cover change, but we actually could have done the tx with no change (which ecash-lib will already figure out)?

It should be handled better, agreed. Either adding utxo selection methods to ecash-lib or making this part of the tx builder. But if it will be a bit unfinished here no matter what, I don't know if we get enough impact from adding another estimator here as a sort of scaffolding, planning to drop it later anyway.

It is probably "less expensive" to not sign twice, but this is not something that will be automated and happening all the time in Cashtab. Weird edge case where we sign more than once at all (we only build the tx once input value exceeds what we need for output, so would only get this where we happen to be "just over the line" and do have enough to cover outputs but do not have enough to cover outputs + fee).

potentially tho I just don't really understand how to do this and making the estimate is easier than I realize.

There's other errors that may be thrown here (e.g. missing SignData), so IMO you should check if the error starts with "Insufficient input value", and let it bubble up otherwise.

good catch, updated

211 ↗(On Diff #47962)

yes. At the moment, it can only be of length 0 or 1. But var name should be plural as it is an array and may be expanded later.

bytesofman marked an inline comment as done.

update param name, more specific error handling in sendXec

is there a way to temporarily deploy this to test it out in actual cashtab? it seems basically gtg

cashtab/src/transactions/index.js
251 ↗(On Diff #47974)

this makes me wonder if you either apply this transformation to all items in the array, or make it scriptOutput (maybe better opreturnOutput ?) and optional (in ts: TxOutput | undefined), instead of a 0 or 1 sized array

This revision now requires changes to proceed.May 29 2024, 15:35

is there a way to temporarily deploy this to test it out in actual cashtab? it seems basically gtg

deployed to https://cashtab-local-dev.netlify.app/

bytesofman marked an inline comment as done.
bytesofman added inline comments.
cashtab/src/transactions/index.js
251 ↗(On Diff #47974)

Yes, imo there are improvement opportunities here. I did not implement in this diff as trying to limit scope as much as possible to just "utxo-lib out for txs, ecash-lib in" -- tho ofc even with this, there are still some cascading changes.

Right now, the getTargetOutput(s) functions in Cashtab are not standardized. Some return an array of multiple outputs, some an array of one output, others an object.

These should all be standardized. Since these functions will also need to be refactored to better support ecash-lib (might as well prepare the target outputs so they are ready for ecash-lib and not so they must be modified in the sendXec function by converting addresses to scripts) -- and this is already tracked in T3582 -- imo okay to leave this existing issue for this planned improvement.

one nit, otherwise gtg

cashtab/package-lock.json
135 ↗(On Diff #47974)

This means it's in node_modules but not in package.json—but it should be removed entirely

Fabien added inline comments.
cashtab/package.json
47 ↗(On Diff #47974)

Out of scope, but this is something we could implement, it doesn't really require to add a new dependency for such a simple thing

cashtab/src/transactions/fixtures/vectors.js
320 ↗(On Diff #47974)

This one is not updated ?

contrib/teamcity/build-configurations.yml
925 ↗(On Diff #47974)

Macro likestamp:

This revision is now accepted and ready to land.May 30 2024, 07:35
bytesofman marked 5 inline comments as done.

version bump, update dummy rawtx in test, clean up package-lock.json

Tail of the build log:

    Finished release-wasm [optimized] target(s) in 5.80s
/work/modules/ecash-lib /work/modules/ecash-lib-wasm /work/modules/ecash-script /work/modules/chronik-client /work/modules/mock-chronik-client /work/modules/ecashaddrjs /work/abc-ci-builds/cashtab-tests

added 362 packages, and audited 364 packages in 2s

60 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

> ecash-lib@0.1.1 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

/work/cashtab /work/modules/ecash-lib /work/modules/ecash-lib-wasm /work/modules/ecash-script /work/modules/chronik-client /work/modules/mock-chronik-client /work/modules/ecashaddrjs /work/abc-ci-builds/cashtab-tests
npm WARN deprecated inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm WARN deprecated @babel/plugin-proposal-private-methods@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-private-methods instead.
npm WARN deprecated @babel/plugin-proposal-class-properties@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
npm WARN deprecated @babel/plugin-proposal-numeric-separator@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-numeric-separator instead.
npm WARN deprecated @babel/plugin-proposal-nullish-coalescing-operator@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-nullish-coalescing-operator instead.
npm WARN deprecated rimraf@3.0.2: Rimraf versions prior to v4 are no longer supported
npm WARN deprecated @babel/plugin-proposal-optional-chaining@7.21.0: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead.
npm WARN deprecated glob@7.2.3: Glob versions prior to v9 are no longer supported
npm WARN deprecated stable@0.1.8: Modern JS already guarantees Array#sort() is a stable sort, so this library is deprecated. See the compatibility table on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility
npm WARN deprecated sourcemap-codec@1.4.8: Please use @jridgewell/sourcemap-codec instead
npm WARN deprecated rollup-plugin-terser@7.0.2: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-terser
npm WARN deprecated workbox-cacheable-response@6.6.0: workbox-background-sync@6.6.0
npm WARN deprecated workbox-google-analytics@6.6.0: It is not compatible with newer versions of GA starting with v4, as long as you are using GAv3 it should be ok, but the package is not longer being maintained
npm WARN deprecated domexception@4.0.0: Use your platform's native DOMException instead
npm WARN deprecated abab@2.0.6: Use your platform's native atob() and btoa() methods instead
npm WARN deprecated @babel/plugin-proposal-private-property-in-object@7.21.11: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-private-property-in-object instead.

added 1651 packages, and audited 1656 packages in 16s

267 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

> cashtab@2.40.0 build
> node scripts/build.js

Creating an optimized production build...

Treating warnings as errors because process.env.CI = true.
Most CI servers set it automatically.

Failed to compile.

Module not found: Error: Can't resolve 'vm' in '/work/cashtab/node_modules/asn1.js/lib/asn1'
BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "vm": require.resolve("vm-browserify") }'
	- install 'vm-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "vm": false }


Build cashtab-tests failed with exit code 1
cashtab/package-lock.json
135 ↗(On Diff #47974)

another unfortunate JS mgmt thing.

ran this to update

rm rf node_modules/
rm package-lock.json
npm i
cashtab/package.json
47 ↗(On Diff #47974)

true, imo it should be added to ecash-lib.

Even then, we might not really need it. It's only necessary in this diff because Cashtab stores private keys in WIF format, since this is what utxo-lib needed to sign with. I did not want to combine a wallet migration with this diff.

As utxo-lib deprecation is completed, would make sense to migrate private key storage to ecash-lib expectation, and then lose this dependency.

For now -- it's only 4kb, very simple lib that is basically just a function. Simplifies implementing ecash-lib.

https://github.com/bitcoinjs/wif/blob/master/index.js

cashtab/src/transactions/fixtures/vectors.js
320 ↗(On Diff #47974)

🤔

I can tell that this test does not get to the point of making this tx due to the thrown error, but I am not sure why this tx was originally included at all.

oh ok. Looking into it -- some of the thrown error tests will expect a tx to be built. Others don't

For the ones that do not -- this tx is just copy pasted as a dummy tx. You can see that this is the same as the preceding test vector.

Updated so dummy tx is more clear.

rerunning npm i rolled in a bunch of dep changes (upgrades) that should be isolated from this diff

back out unintended package-lock dep upgrades

This revision is now accepted and ready to land.May 30 2024, 19:23