Add multisig support to ecash-lib. This will make it easier to, for example, use multisig for token txs.
Details
Details
- Reviewers
tobias_ruck - Group Reviewers
Restricted Project - Commits
- rABCc73c75cc1444: [ecash-lib] Support bare and p2sh-wrapped multisig with ECDSA and Schnorr…
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| modules/ecash-lib/src/script.ts | ||
|---|---|---|
| 196 ↗ | (On Diff #58558) | I see this is the C++ code? static constexpr int MAX_PUBKEYS_PER_MULTISIG = 20; Also a good idea to put it as a constant into ecash-lib |
| 219 ↗ | (On Diff #58558) | maybe it makes more sense to turn the params into an object |
| 222 ↗ | (On Diff #58558) | |
| 223 ↗ | (On Diff #58558) | let's just use easy to understand names, no need to use Core's or whatever "simplified" terminology |
| 280–281 ↗ | (On Diff #58558) | |
| 373 ↗ | (On Diff #58558) | is there a way to use this function within parseP2shMultisigSpend? I think you just need to extract the redeem script out and then have that as your outputScript. Cuts down a lot on the code duplication |
| 482 ↗ | (On Diff #58558) | could be a private method in Script taking this, but either is fine with me |
Comment Actions
correct and define constant to match c++, improve variable names, params object instead of params, refactor for less code duplication in parsing
| modules/ecash-lib/src/script.test.ts | ||
|---|---|---|
| 282 ↗ | (On Diff #58559) | Is there a way to break this up into multiple lines? Maybe using back ticks? |
| 291 ↗ | (On Diff #58559) | Do a deep equal on the actual array. signatures[0] could be anything |
| 298 ↗ | (On Diff #58559) | Also do a deep equal on the pubkeys array, its not tested at all |
| 299 ↗ | (On Diff #58559) | Ideally do a deep equal on the entire parsed so we don’t miss anything, but that may be cumbersome |
| 315 ↗ | (On Diff #58559) | Here too multiple lines would be great |
| 322 ↗ | (On Diff #58559) | parsed.pubkeys is untested and signatures basically too |
| 336 ↗ | (On Diff #58559) | And here |
| modules/ecash-lib/src/script.ts | ||
| 199 ↗ | (On Diff #58559) | Hmm better to split this up into 3 different error messages, makes it easier to understand the error and the code is easier to read too |
| 283 ↗ | (On Diff #58559) | |
| 460 ↗ | (On Diff #58559) | One thing to consider here is the case of a partially signed input, in that case it may not be known yet which pubkeys will sign in the future, and therefore the pubkey indices may not be known |
| modules/ecash-lib/src/txBuilder.ts | ||
| 236 ↗ | (On Diff #58559) | Let’s just put this in another diff just for my sanity Nothing bad can happen, it can only good can happen |
| modules/ecash-lib/src/script.ts | ||
|---|---|---|
| 460 ↗ | (On Diff #58559) | For now I just added some docs to reflect this. In practice I'm not familiar enough with multisig expected use to call the best way to handle this. Is there a different behavior you think would be more appropriate? |
| 196 ↗ | (On Diff #58558) | TIL |
| 219 ↗ | (On Diff #58558) | indeed, esp with 2 optional like this |
| 482 ↗ | (On Diff #58558) | I don't see this being generally used, so imo makes sense to keep out of the class for now --- though maybe in the future we could support something like a Script.parse() that returns various private methods |
| modules/ecash-lib/src/txBuilder.ts | ||
| 236 ↗ | (On Diff #58559) | k -- useful for me to confirm this stuff works at all in the tests, but I don't mind splitting it out |
Comment Actions
back out tx builder changes, improve test expectations, improve error msgs, respond to feedback
Comment Actions
It's probably a good idea to add a test file along side txBuilder.test.ts, something like multisig.test.ts where we test these functions.
We don't need to add any actual signatory functions, just set the signatory for the multisig inputs to an arrow function that references all the private keys and creates a corresponding multisig spend (the UnsignedTxInput has everything you need), like this:
inputs: utxos.utxos.map(utxo => ({
input: {
prevOut: utxo.outpoint,
signData: {
...
},
},
signatory: (ecc, input) => {
const preimage = input.sigHashPreimage(sigHashType);
const sighash = sha256d(preimage.bytes);
const signatures = sks.map(sk => signWithSigHash(ecc, sk, sighash, ALL_BIP143));
return Script.multisigSpend({ signatures, redeemScript: preimage.redeemScript });
},
})),| modules/ecash-lib/src/script.test.ts | ||
|---|---|---|
| 331 ↗ | (On Diff #58576) | any reason why this is longer than the other tests? |
| modules/ecash-lib/src/script.ts | ||
| 460 ↗ | (On Diff #58559) | One option would be to not throw this error if any signatures are undefined |
| 482 ↗ | (On Diff #58558) | well it's not that it's supposed to be generally used, just this way it stays closer to the caller |
| modules/ecash-lib/src/txBuilder.ts | ||
| 236 ↗ | (On Diff #58559) | just too many moving parts |
| modules/ecash-lib/src/script.test.ts | ||
|---|---|---|
| 331 ↗ | (On Diff #58576) | no, changed to be comparable to the others. |
| modules/ecash-lib/src/script.ts | ||
| 482 ↗ | (On Diff #58558) | rationale here
so, these users would prob not even include this function in their bundle if their app were not using multisig. but if this method were defined as a private method in Script, then all ecash-lib users would always include it in their app, even tho they may not be using it. |
| modules/ecash-lib/src/script.ts | ||
|---|---|---|
| 460 ↗ | (On Diff #58559) | ok I get this now, removed, much better that way |
| modules/ecash-lib/src/script.ts | ||
|---|---|---|
| 482 ↗ | (On Diff #58558) | oh so you're saying that bundlers are too dumb to cut out parseMultisigRedeemScript if it's a method of Script, whereas if it's standalone they can cut it out? if that's true then fine, but I doubt it tbh... anyway |
| modules/ecash-lib/src/script.ts | ||
|---|---|---|
| 482 ↗ | (On Diff #58558) | hm thinking about this I really have no idea. it seems to be true tho https://grok.com/share/c2hhcmQtMi1jb3B5_7fc253d5-ab00-4a0f-aa53-858b58c9cfeb |
| modules/ecash-lib/src/script.ts | ||
|---|---|---|
| 349 ↗ | (On Diff #58612) | d'oh
fixed |
| modules/ecash-lib/src/script.ts | ||
|---|---|---|
| 313 ↗ | (On Diff #58619) | make sure to test for some of the failure modes of this method in the tests |