Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spend to Taproot #91

Merged
merged 56 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
8b72e86
fix: don't use polyfills for process.env
bucko13 May 1, 2024
6b8f3bc
cleanup deps
bucko13 May 1, 2024
dcfc0ec
remove unused legacy psbt module
bucko13 May 1, 2024
4ed5e74
updated psbtv0 tx creator
bucko13 May 2, 2024
a826fff
add vendor-ized tiny-secp25k1 with buffer fix
bucko13 May 3, 2024
801bc78
fix: jest and other test cleanups
bucko13 May 3, 2024
dacc5eb
feat: moves some constants from c/bitcoin to c/psbt
bucko13 May 3, 2024
ca8a8b3
chore: cleanup
bucko13 May 3, 2024
bce2d08
feat: init ecc for taproot output support and taproot address test
bucko13 May 3, 2024
fda1072
chore: cleanup readme
bucko13 May 3, 2024
36b2530
Merge branch 'main' into taproot-spends
bucko13 May 3, 2024
09e5a67
fix: need node polyfills for caravan/psbt
bucko13 May 3, 2024
698091f
fix: tests and legacy psbts should respect network for global xpubs
bucko13 May 3, 2024
0df5bc6
fix: refactor to make psbt conversions easier with legacy api
bucko13 May 6, 2024
6b987c3
fix: cleanup package.jsons for importing caravan/psbt into coordinator
bucko13 May 6, 2024
32fbd9b
chore: readme details about jest/lib-secp oddities
bucko13 May 6, 2024
01ff296
cleanup dependencies and use bitcoinjs-lib alias in caravan/psbt
bucko13 May 7, 2024
77e177e
consolidate secp256k1 vendor files to just commit to asmjs package
bucko13 May 7, 2024
575aeeb
fix function names
bucko13 May 7, 2024
404e160
chore: prettier cleanups
bucko13 May 7, 2024
82f0ea3
fix: networkData should support regtest
bucko13 May 7, 2024
9d5f8da
chore: cleanups to turbo generator templates
bucko13 May 7, 2024
8235f8d
feat: add new package for handing caravan multisig config data
bucko13 May 7, 2024
14c6b76
fix: update dependencies to use new multisig package
bucko13 May 7, 2024
fd6e7c8
chore: added TODOs for supporting spending to taproot addresses
bucko13 May 7, 2024
2b8c899
more dependency cleanup
bucko13 May 7, 2024
eed8801
add multisig tests
bucko13 May 7, 2024
1dc8d13
fix: test and ci cleanup
bucko13 May 7, 2024
223efc9
revert: bitcoinjs-lib upgrade in coordinator
bucko13 May 7, 2024
85528b7
Merge branch 'main' into taproot-spends
bucko13 May 7, 2024
7550bd1
more info on npm aliasing
bucko13 May 7, 2024
9e4a19e
nit
bucko13 May 7, 2024
f2e06a7
jest restore all mocks
bucko13 May 9, 2024
b426ae1
PR cleanup
bucko13 May 9, 2024
8226c95
chore: replace old type for legacy inputs
bucko13 May 21, 2024
15bb709
chore: move bufferize to module level functions
bucko13 May 21, 2024
4088c6e
add signature validation for psbt v0
bucko13 May 22, 2024
0c969cc
export signature validation from caravan/bitcoin and use instead of n…
bucko13 May 22, 2024
29ac63b
fixes for test configurations
bucko13 May 23, 2024
e098a8b
better regtest support
bucko13 May 23, 2024
8cab8d3
make sure caravan/bitcoin is pinned to legacy bitcoinjs-lib
bucko13 May 23, 2024
3280982
dependency cleanup
bucko13 May 23, 2024
83a81c4
feat: use new tx processing code for s2tr support
bucko13 May 23, 2024
185c261
ci cleanup
bucko13 May 23, 2024
3b8b75f
Merge branch 'main' into taproot-spends
bucko13 May 23, 2024
80fbd51
Merge branch 'main' into taproot-spends
bucko13 May 23, 2024
43f4f99
cleanup
bucko13 Jun 3, 2024
9183567
upgrading rest of caravan/wallets impl to use new caravan/psbt utils
bucko13 Jun 3, 2024
f2768ca
add changesets
bucko13 Jun 3, 2024
4f17031
lint fix
bucko13 Jun 3, 2024
e6a6b45
fix: tests and throw for missing inputs for coldcard
bucko13 Jun 3, 2024
37d46d6
change version change to caravan/bitcoin
bucko13 Jun 3, 2024
4b8c95e
testing cleanup
bucko13 Jun 3, 2024
f0486bf
absolute import for src/functions
bucko13 Jun 3, 2024
81c17fa
test for improper encoding
bucko13 Jun 3, 2024
24c5c5b
fix: validation error for regtest
bucko13 Jun 6, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/calm-adults-own.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@caravan/bitcoin": minor
---

export signature utilities from caravan/bitcoin to support new psbt tooling
5 changes: 5 additions & 0 deletions .changeset/light-plants-remember.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@caravan/bitcoin": major
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change must be a major version increment? I was hoping each major version release could consolidate issues requiring a major version increment such as this one: #59

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was conflicted about this. Basically I decided a major version was necessary because the psbts being created are going to be different now (but not invalid). Perhaps minor is justifiable for this.

I don't think it makes sense to lump other changes into this already giant PR. But if we wanted we could have other PRs that follow on this before we actually merge the version bump PRs that are created by the automation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to avoid major increments for small things if we're trying to encourage more developers to build using this library. I think we should collect a bunch of breaking changes and add them all at once.

As for the differing psbts, I don't believe that's a breaking change. The psbts have their own BIP defined required and optional values. You're allowed to not include an optional value (if that's what's being removed here) and consumers were supposed to have developed for that possibility. I think the only way this would have been breaking is if somewhere in the spec/docs of this library that value was guaranteed.

---

transaction parser was stripping out network information from global xpubs being added to psbt. global xpubs will now respect the network and include appropriate prefix
6 changes: 6 additions & 0 deletions .changeset/twelve-kids-provide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@caravan/wallets": patch
"caravan-coordinator": patch
---

fixes an issue where esbuild was polyfilling process.env which breaks the ability to override trezor connect settings and pass other env vars at run time in dependent applications
4 changes: 4 additions & 0 deletions apps/coordinator/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ By default, Caravan uses a free API provided by
information about the bitcoin blockchain or to broadcast transactions. Blockstream.info is also available as a fallback
option for a public API.

Mainnet and Testnet are available options for connecting to any of the available
consensus client options. Regtest can be available through an uploaded wallet
configuration file, but only for the private client backend.

### Bitcoind client

You can also ask Caravan to use your own private [bitcoind full
Expand Down
25 changes: 0 additions & 25 deletions apps/coordinator/jest.config.json

This file was deleted.

27 changes: 27 additions & 0 deletions apps/coordinator/jest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import type { JestConfigWithTsJest } from "ts-jest";

const config: JestConfigWithTsJest = {
testEnvironment: "jsdom",
transform: {
"\\.[jt]sx?$": "babel-jest",
},
transformIgnorePatterns: ["^.+\\.module\\.(css|sass|scss)$"],
moduleNameMapper: {
"^.+\\.module\\.(css|sass|scss)$": "identity-obj-proxy",
},
moduleFileExtensions: [
"web.cjs",
"js",
"web.ts",
"ts",
"web.tsx",
"tsx",
"json",
"web.cjsx",
"jsx",
"node",
],
setupFilesAfterEnv: ["<rootDir>/jest.setup.ts"],
};

export default config;
1 change: 1 addition & 0 deletions apps/coordinator/jest.setup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import "@inrupt/jest-jsdom-polyfills";
4 changes: 3 additions & 1 deletion apps/coordinator/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"@babel/preset-env": "^7.20.2",
"@babel/preset-react": "^7.18.6",
"@babel/preset-typescript": "^7.21.0",
"@inrupt/jest-jsdom-polyfills": "^3.2.1",
"@testing-library/jest-dom": "^5.6.0",
"@testing-library/react": "^10.0.4",
"@types/history": "^5.0.0",
Expand Down Expand Up @@ -95,8 +96,9 @@
"dependencies": {
"@caravan/bitcoin": "*",
"@caravan/clients": "*",
"@caravan/descriptors": "^0.0.6",
"@caravan/descriptors": "^0.1.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update to support regtest xpubs

"@caravan/eslint-config": "*",
"@caravan/psbt": "*",
"@caravan/typescript-config": "*",
"@caravan/wallets": "*",
"@emotion/react": "^11.10.6",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import PropTypes from "prop-types";
import { connect } from "react-redux";
import {
validateHex,
validateMultisigSignature,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is one of three legacy functions that needed to be replaced.

multisigBIP32Path,
multisigBIP32Root,
validateBIP32Path,
Expand Down Expand Up @@ -38,6 +37,12 @@ import {
} from "../../actions/signatureImporterActions";
import { setSigningKey as setSigningKeyAction } from "../../actions/transactionActions";
import { downloadFile } from "../../utils";
import {
convertLegacyInput,
convertLegacyOutput,
getUnsignedMultisigPsbtV0,
validateMultisigPsbtSignature,
} from "@caravan/psbt";

const TEXT = "text";
const UNKNOWN = "unknown";
Expand Down Expand Up @@ -389,12 +394,17 @@ class SignatureImporter extends React.Component {

let publicKey;
try {
publicKey = validateMultisigSignature(
const args = {
network,
inputs,
outputs,
inputs: inputs.map(convertLegacyInput),
outputs: outputs.map(convertLegacyOutput),
};
const psbt = getUnsignedMultisigPsbtV0(args);
publicKey = validateMultisigPsbtSignature(
psbt.toBase64(),
inputIndex,
inputSignature,
inputs[inputIndex].amountSats,
);
} catch (e) {
errback(`Signature for input ${inputNumber} is invalid.`);
Expand Down Expand Up @@ -482,13 +492,17 @@ class SignatureImporter extends React.Component {
return;
}
try {
// This returns false if it completes with no error
publicKey = validateMultisigSignature(
const args = {
network,
inputs,
outputs,
inputs: inputs.map(convertLegacyInput),
outputs: outputs.map(convertLegacyOutput),
};
const psbt = getUnsignedMultisigPsbtV0(args);
publicKey = validateMultisigPsbtSignature(
psbt.toBase64(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a big change in the API. The main functions now operating past the creator role should take a psbt and can extract inputs/output data from there.

inputIndex,
inputSignature,
inputs[inputIndex].amountSats,
);
} catch (e) {
// eslint-disable-next-line no-console
Expand Down Expand Up @@ -579,7 +593,8 @@ SignatureImporter.propTypes = {
}),
).isRequired,
fee: PropTypes.string.isRequired,
inputs: PropTypes.arrayOf(PropTypes.shape({})).isRequired,
inputs: PropTypes.arrayOf(PropTypes.shape({ amountSats: PropTypes.string }))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it actually has mre than this but this value is directly used and so an error was being thrown for not having it declared. I didn't spend too much time building out the proptypes as typescript will obsolete this.

.isRequired,
inputsTotalSats: PropTypes.shape({}).isRequired,
isWallet: PropTypes.bool.isRequired,
network: PropTypes.string.isRequired,
Expand Down
49 changes: 36 additions & 13 deletions apps/coordinator/src/components/ScriptExplorer/Transaction.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ import React from "react";
import PropTypes from "prop-types";
import { connect } from "react-redux";
import {
signedMultisigTransaction,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a second function that's replaced (it uses unsignedMultisigTransaction, one of the others that needed to be upgraded)

blockExplorerTransactionURL,
Network,
addSignaturesToPSBT,
} from "@caravan/bitcoin";

import {
Expand All @@ -21,6 +22,13 @@ import { updateBlockchainClient } from "../../actions/clientActions";
import Copyable from "../Copyable";
import { externalLink } from "utils/ExternalLink";
import { setTXID } from "../../actions/transactionActions";
import {
convertLegacyInput,
convertLegacyOutput,
getUnsignedMultisigPsbtV0,
} from "@caravan/psbt";
import { Psbt } from "bitcoinjs-lib";
import { Buffer } from "buffer";

class Transaction extends React.Component {
constructor(props) {
Expand All @@ -34,14 +42,30 @@ class Transaction extends React.Component {

buildSignedTransaction = () => {
const { network, inputs, outputs, signatureImporters } = this.props;
return signedMultisigTransaction(
const args = {
network,
inputs,
outputs,
Object.values(signatureImporters).map(
(signatureImporter) => signatureImporter.signature,
),
);
inputs: inputs.map(convertLegacyInput),
outputs: outputs.map(convertLegacyOutput),
};
const psbt = getUnsignedMultisigPsbtV0(args);
let partiallySignedTransaction = psbt.toBase64();
for (const signatureImporter of Object.values(signatureImporters)) {
partiallySignedTransaction = addSignaturesToPSBT(
Network.REGTEST,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: don't hardcode network @bucko13

partiallySignedTransaction,
signatureImporter.publicKeys.map((pubkey) =>
Buffer.from(pubkey, "hex"),
),
signatureImporter.signature.map((signature) =>
Buffer.from(signature, "hex"),
),
);
}

return Psbt.fromBase64(partiallySignedTransaction)
.finalizeAllInputs()
.extractTransaction()
.toHex();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the newer versions of these functions, I opted to make them less "magical" and extract some of the logic that was happening under the hood. Previously, signedMultisigTransaction would take inputs, outputs, and signatures and then create a PSBT and then add the various signatures for you. IMO this abstracts away the PSBT saga too much and then makes the functions tightly coupled to a specific API and expected saga flow.

The new approach is to expect the consumer of these functions to just add the signature it needs to add to a given PSBT (addSignaturesToPsbt already existed and didn't need to be upgraded). So this change first creates the PSBT (ideally we're just passing around PSBTs at this point so this step can be skipped), then finds each pubkey and signature to add to the psbt. the legacy signedMultisigTransaction did this all "automatically"

Additionally, since this component method buildSignedTransaction is already building the finalized transaction, it can just return the raw hex.

};

handleBroadcast = async () => {
Expand All @@ -52,7 +76,7 @@ class Transaction extends React.Component {
let txid = "";
this.setState({ broadcasting: true });
try {
txid = await client.broadcastTransaction(signedTransaction.toHex());
txid = await client.broadcastTransaction(signedTransaction);
} catch (e) {
// eslint-disable-next-line no-console
console.error(e);
Expand All @@ -71,14 +95,13 @@ class Transaction extends React.Component {

render() {
const { error, broadcasting, txid } = this.state;
const signedTransaction = this.buildSignedTransaction();
const signedTransactionHex = signedTransaction.toHex();
const signedTransactionHex = this.buildSignedTransaction();
return (
<Card>
<CardHeader title="Broadcast" />
<CardContent>
<form>
{signedTransaction && (
{signedTransactionHex && (
<Box mt={4}>
<Typography variant="h6">Signed Transaction</Typography>
<Copyable text={signedTransactionHex} code showIcon />
Expand All @@ -89,7 +112,7 @@ class Transaction extends React.Component {
<Button
variant="contained"
color="primary"
disabled={!signedTransaction || broadcasting}
disabled={!signedTransactionHex || broadcasting}
onClick={this.handleBroadcast}
>
Broadcast Transaction
Expand Down
6 changes: 1 addition & 5 deletions apps/coordinator/src/components/Wallet/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,7 @@ class CreateWallet extends React.Component {
setTotalSigners(walletConfiguration.quorum.totalSigners);
setRequiredSigners(walletConfiguration.quorum.requiredSigners);
setAddressType(walletConfiguration.addressType);
if (walletConfiguration.network === "regtest") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can actually support regtest now.

setNetwork("testnet");
} else {
setNetwork(walletConfiguration.network);
}
setNetwork(walletConfiguration.network);
updateWalletNameAction(0, walletConfiguration.name);
updateWalletUuid(walletConfiguration.uuid);

Expand Down
26 changes: 16 additions & 10 deletions apps/coordinator/src/reducers/transactionReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@ import {
satoshisToBitcoins,
bitcoinsToSatoshis,
validateAddress,
unsignedMultisigTransaction,
unsignedMultisigPSBT,
unsignedTransactionObjectFromPSBT,
checkFeeRateError,
getFeeErrorMessage,
FeeValidationError,
unsignedMultisigTransaction,
} from "@caravan/bitcoin";
import {
convertLegacyInput,
convertLegacyOutput,
getUnsignedMultisigPsbtV0,
} from "@caravan/psbt";
import updateState from "./utils";
import { SET_NETWORK, SET_ADDRESS_TYPE } from "../actions/settingsActions";
import {
Expand Down Expand Up @@ -45,6 +48,7 @@ import {
SPEND_STEP_CREATE,
} from "../actions/transactionActions";
import { RESET_NODES_SPEND } from "../actions/walletActions";
import { Transaction } from "bitcoinjs-lib";

function sortInputs(a, b) {
const x = a.txid.toLowerCase();
Expand Down Expand Up @@ -280,16 +284,18 @@ function finalizeOutputs(state, action) {
// First try to build the transaction via PSBT, if that fails (e.g. an input doesn't know about its braid),
// then try to build it using the old TransactionBuilder plumbing.
try {
const unsignedTransactionPSBT = unsignedMultisigPSBT(
state.network,
state.inputs,
state.outputs,
);
unsignedTransaction = unsignedTransactionObjectFromPSBT(
unsignedTransactionPSBT,
const args = {
network: state.network,
inputs: state.inputs.map(convertLegacyInput),
outputs: state.outputs.map(convertLegacyOutput),
};
const psbt = getUnsignedMultisigPsbtV0(args);
unsignedTransaction = Transaction.fromHex(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another example of not needing a specialized function. If we have a psbt, it has the unsignedTx on its globalMap (if v0) so just use that.

psbt.data.globalMap.unsignedTx.toBuffer().toString("hex"),
);
} catch (e) {
// probably has an input that isn't braid aware.
// NOTE: This won't work for txs with taproot outputs
unsignedTransaction = unsignedMultisigTransaction(
state.network,
state.inputs,
Expand Down
1 change: 1 addition & 0 deletions apps/coordinator/src/reducers/transactionReducer.test.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import "@testing-library/jest-dom";
import BigNumber from "bignumber.js";
import {
P2WSH,
Expand Down
7 changes: 2 additions & 5 deletions apps/coordinator/src/tests/addresses.jsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
import React from "react";

import { blockExplorerAddressURL, TEST_FIXTURES } from "@caravan/bitcoin";
import {
ConfirmMultisigAddress,
LEDGER,
braidDetailsToWalletConfig,
} from "@caravan/wallets";
import { ConfirmMultisigAddress, LEDGER } from "@caravan/wallets";
import { Box, Table, TableBody, TableRow, TableCell } from "@mui/material";
import { externalLink } from "utils/ExternalLink";

import Test from "./Test";
import { braidDetailsToWalletConfig } from "@caravan/multisig";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few exports were moved to this new internal package (internal because it's not published and only marked as private for now) to avoid circular import issues between packages. This was marked as a TODO anyway in the code (I'll comment on the relevant updates when I get to those files)


class ConfirmMultisigAddressTest extends Test {
name() {
Expand Down
6 changes: 2 additions & 4 deletions apps/coordinator/src/tests/registration.jsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import React from "react";

import { TEST_FIXTURES } from "@caravan/bitcoin";
import {
RegisterWalletPolicy,
braidDetailsToWalletConfig,
} from "@caravan/wallets";
import { RegisterWalletPolicy } from "@caravan/wallets";
import { Box, Table, TableBody, TableRow, TableCell } from "@mui/material";

import Test from "./Test";
import { braidDetailsToWalletConfig } from "@caravan/multisig";

class RegisterWalletPolicyTest extends Test {
name() {
Expand Down
2 changes: 1 addition & 1 deletion apps/coordinator/src/tests/signing.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ import {
HERMIT,
LEDGER,
SignMultisigTransaction,
braidDetailsToWalletConfig,
} from "@caravan/wallets";
import { Box, Table, TableBody, TableRow, TableCell } from "@mui/material";
import { externalLink } from "utils/ExternalLink";
import Test from "./Test";
import { braidDetailsToWalletConfig } from "@caravan/multisig";

class SignMultisigTransactionTest extends Test {
name() {
Expand Down
Loading
Loading