Multi Sig Transactions #109

reedrosenbluth · 2020-08-05T15:10:39Z

Description

This PR adds support for building Multi-Sig transactions.

Issue: #8

TODO

fix AuthField serialization bug
remove bitcoin-js dependency
add multi-sig tx builder
implement remaining hash modes (P2WPK-P2SH and P2WSH-P2SH)
- can do this in a followup PR...
double check that auth clearing works as described: https://github.com/blockstack/stacks-blockchain/blob/master/sip/sip-005-blocks-and-transactions.md#transaction-signing-and-verifying
more test vectors
merge master

Type of Change

New feature
Bug fix
API reference/documentation update
Other

Does this introduce a breaking change?

List the APIs or describe the functionality that this PR breaks.
Workarounds for or expected timeline for deprecation

Are documentation updates required?

Testing information

Provide context on how tests should be performed.

Is testing required for this change?
If it’s a bug fix, list steps to reproduce the bug
Briefly mention affected code paths
List other affected projects if possible
Things to watch out for when testing

Checklist

Code is commented where needed
Unit test coverage for new or modified code paths
npm run test passes
Changelog is updated
Tag 1 of @yknl, @zone117x, @reedrosenbluth for review

…tion from class to interface

codecov · 2020-08-11T19:18:53Z

Codecov Report

Merging #109 into master will increase coverage by 1.15%.
The diff coverage is 86.44%.

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   82.76%   83.91%   +1.15%     
==========================================
  Files          28       27       -1     
  Lines        2031     2052      +21     
  Branches      438      416      -22     
==========================================
+ Hits         1681     1722      +41     
+ Misses        346      326      -20     
  Partials        4        4

Impacted Files	Coverage Δ
src/types.ts	`81.66% <60.00%> (-1.52%)`	⬇️
src/builders.ts	`75.26% <67.74%> (-1.98%)`	⬇️
src/signer.ts	`74.57% <73.33%> (+1.36%)`	⬆️
src/transaction.ts	`79.43% <90.00%> (+1.34%)`	⬆️
src/authorization.ts	`86.11% <91.27%> (+8.28%)`	⬆️
src/utils.ts	`97.46% <94.44%> (+17.70%)`	⬆️
src/constants.ts	`100.00% <100.00%> (ø)`
src/keys.ts	`90.42% <100.00%> (-0.11%)`	⬇️
src/index.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29f23dc...ff911cf. Read the comment docs.

zone117x · 2020-08-12T21:33:53Z

src/utils.ts

+
+// Internally, the Stacks blockchain encodes address the same as Bitcoin
+// multi-sig address (p2sh)
+export const hashP2SH = (numSigs: number, pubKeys: Buffer[]): string => {


No bitcoinjs-lib dependency? 💯

Great work getting this to work without bitcoinjs-lib 👍

zone117x · 2020-08-12T21:35:25Z

tests/src/authorization-tests.ts

-test('Single sig wpkh spending condition compressed', () => {
-  const addressHashMode = AddressHashMode.SerializeP2WPKH;
-  const nonce = new BigNum(345);
+test('Multi sig spending condition uncompressed', () => {


Was the original test Single sig wpkh spending condition compressed intentionally removed?

It was. It turns out that WPKH wasn't implemented yet. I'm not actually sure why that test was passing originally, but after my refactor it began to fail (with the expected Unimplemented Error) so I removed it

zone117x · 2020-08-12T21:37:33Z

tests/src/authorization-tests.ts

  ];
+
  const spendingConditionBytes = Buffer.from(spendingConditionBytesHex);


Are there any test vectors copied from the rust codebase to ensure consistency?

The multi-sig test in transaction-tests.js is mostly copied from the rust codebase, but I'm planning on adding some more test vectors. Will try and add some to this PR tomorrow

zone117x · 2020-08-12T21:40:30Z

Love how clean this PR is. Code looks good. Before I approve I'd like to use this functionality in something (probably with a sidecar tx debug endpoint) for a good end-to-end test.

yknl

This looks good! Would love to see a builder function or a series of functions that help make the signing process easier.

src/builders.ts

kyranjamie · 2020-08-18T10:08:36Z

src/builders.ts

@@ -1,3 +1,5 @@
+import * as _ from 'lodash';


Are you sure this doesn't include the entire lodash bundle?

kyranjamie · 2020-08-18T10:11:26Z

src/builders.ts

+  let spendingCondition = null;
+
+  if ('publicKey' in options) {
+    // multi-sig


this would not be multi-sig case, right?

yup, this is a typo!

yknl · 2020-08-19T21:21:58Z

This looks good to merge 👍

zone117x · 2020-08-19T23:03:02Z

There's a regression with sponsored transaction signing/serialization. Here's some data I collected to help debug.

A contract-call transaction's serialized payload:

// from this branch (this is an invalid tx):
80800000000500164247d6f2b425ac5771423ae6c80c754f7172b0000000000000000100000000000030390000d357ba68874c8d2b67c497c14af2e95f726b9e21f83f624acccfca20d2f15dbf4bf81da24564a550f4e96eef6922723a8d390943996aa300eecc2123b2df9eae0031ef5ee9a226a792b93f2bfbfbc54f523eba781800000000000000000000000000000118000111ed033667fd0ad5d4136455b5d0300d16c8daf496f1ecd09697db8e62164c7247537db17e1b7aa636cd8957d4c504a01d3e63a2b7eb89ff5f7b546351e8687e030100000000021a164247d6f2b425ac5771423ae6c80c754f7172b01468656c6c6f2d776f726c642d636f6e7472616374096765742d76616c7565000000010200000000

// from master branch (this is a valid tx):
80800000000500164247d6f2b425ac5771423ae6c80c754f7172b0000000000000000100000000000030390000d357ba68874c8d2b67c497c14af2e95f726b9e21f83f624acccfca20d2f15dbf4bf81da24564a550f4e96eef6922723a8d390943996aa300eecc2123b2df9eae0031ef5ee9a226a792b93f2bfbfbc54f523eba7818000000000000000000000000000001180000a24c32196623aa72f1d53324ffbf7d203ae8b148697aaef49ff8f809cdcab5e375bb9b84d3d1a0e60073458d9a8999c5b5941c0b248a0aa66b05fcbcc6d5657f030100000000021a164247d6f2b425ac5771423ae6c80c754f7172b01468656c6c6f2d776f726c642d636f6e7472616374096765742d76616c7565000000010200000000

Also, here's the error returned by the core-node when submitting the invalid transaction:

Error: Response 400: Bad Request fetching http://127.0.0.1:20443/v2/transactions:

{
  "error": "transaction rejected",
  "reason": "SignatureValidation",
  "reason_data": {
    "message": "Signer hash does not equal hash of public key(s): 9773d279b98a0e1388875729207fe7602cda7af5 != 31ef5ee9a226a792b93f2bfbfbc54f523eba7818"
  },
  "txid": "4a407703abe75fb5d1ad489875296d9eff52ad64bbf780b44d08d412ccb5faf9"
}

zone117x · 2020-08-20T15:38:03Z

Draft PR in sidecar for supporting multisig transactions, currently helpful as an integration test with core-node hirosystems/stacks-blockchain-api#193

Core-node is rejecting multisig transactions with same error as the sponsored txs:

{
  "error": "transaction rejected",
  "reason": "SignatureValidation",
  "reason_data": {
    "message": "Signer hash does not equal hash of public key(s): fbcc4c8a97d1fe9d96cdf06c98ce05eccbc9ff35 != 55c50b056f6f2c500b4432b9e89828b3c8d607cf"
  },
  "txid": "54878b0cd7511af52d0ea901aa21c1f9f87c3ef254cb08bea8a82d1bb5aa4904"
}

zone117x · 2020-08-21T01:07:03Z

Update: the latest commit fixes multisig transactions -- they are processed by the core-node as expected.

However, the sponsored transaction regression still exists.

reedrosenbluth · 2020-08-21T16:41:37Z

Update: the latest commit fixes multisig transactions -- they are processed by the core-node as expected.

However, the sponsored transaction regression still exists.

I'm able to generate and submit a sponsored transaction with out any issues (on my local mocknet). @zone117x Can you sure some more detail about the transaction that isn't working for you?

zone117x

The sponsored tx regression has been fixed.

Additionally, sponsored + multisig transactions also work 👍

SHIP IT

project-bot bot added this to New issues in Splat Team Aug 5, 2020

agraebe moved this from New issues to In progress in Splat Team Aug 10, 2020

reedrosenbluth added 11 commits August 11, 2020 13:44

refactor: remove extraneous BufferReader class

79bf8c1

build: package-lock

d5fb96e

refactor: hash160 returns buffer instead of string

0d99c47

feat: hashP2SH function for multi-sig addresses

e3d900a

fix: public key deserialization

7aed038

fix: AddressHashMode comments, and introduce new hash mode types

76c2f9a

refactor: MessageSignature from class to interface

d01ae8f

feat: implement MultiSigSpendingCondition, and refactor SpendingCondi…

c4dc124

…tion from class to interface

refactor: rest of lib to work with refactored Auth

977a2c6

feat: MultiSig transaction signing

db15321

test: multisig auth and tx signing

3ebe083

reedrosenbluth force-pushed the feat/multi-sig-gen branch from f70c44f to 3ebe083 Compare August 11, 2020 18:08

Merge branch 'master' into feat/multi-sig-gen

5281ea2

reedrosenbluth marked this pull request as ready for review August 11, 2020 19:19

reedrosenbluth requested review from yknl and zone117x August 11, 2020 19:19

agraebe linked an issue Aug 11, 2020 that may be closed by this pull request

Add multi-sig transaction generation #8

Closed

agraebe removed this from In progress in Splat Team Aug 11, 2020

reedrosenbluth changed the title ~~Multi Sig~~ Multi Sig Transactions Aug 12, 2020

zone117x reviewed Aug 12, 2020

View reviewed changes

yknl approved these changes Aug 13, 2020

View reviewed changes

reedrosenbluth added 3 commits August 13, 2020 17:26

refactor: remove unnecessary version arg and add comment

62e534b

feat: build multi-sig token transfer transactions

335230e

test: multi-sig token transfer transaction builder

88bfe0c

chore: update changelog

3e4be15

yknl reviewed Aug 14, 2020

View reviewed changes

src/builders.ts Outdated Show resolved Hide resolved

fix: make signerKeys optional in tx builder

3ee4ed2

reedrosenbluth force-pushed the feat/multi-sig-gen branch from 1fe713e to 3ee4ed2 Compare August 14, 2020 17:56

refactor: separate builder function for signed and unsigned transactions

694b2dc

reedrosenbluth force-pushed the feat/multi-sig-gen branch from b22f3ea to 694b2dc Compare August 14, 2020 20:41

yknl mentioned this pull request Aug 17, 2020

feat: add ability to create unsigned txs #115

Closed

kyranjamie approved these changes Aug 18, 2020

View reviewed changes

yknl mentioned this pull request Aug 18, 2020

Support for unsigned transactions #112

Closed

agraebe linked an issue Aug 18, 2020 that may be closed by this pull request

Support for unsigned transactions #112

Closed

fix: typos

c6f0eca

fix: makeSigHashPostSign and clarify MessageSignature type

909096f

reedrosenbluth added 3 commits August 24, 2020 15:22

fix: revert sponsored AuthType back to standard

1833326

fix: types import

1a14863

fix: multi-sig builder test

9899c3c

zone117x mentioned this pull request Aug 24, 2020

Sponsored tx origin verification using standard auth flags stacks-network/stacks-core#1826

Closed

zone117x approved these changes Aug 24, 2020

View reviewed changes

Merge branch 'master' into feat/multi-sig-gen

ff911cf

reedrosenbluth merged commit f41adb1 into master Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi Sig Transactions #109

Multi Sig Transactions #109

reedrosenbluth commented Aug 5, 2020 •

edited

codecov bot commented Aug 11, 2020 •

edited

zone117x Aug 12, 2020

yknl Aug 13, 2020

zone117x Aug 12, 2020

reedrosenbluth Aug 12, 2020 •

edited

zone117x Aug 12, 2020

reedrosenbluth Aug 12, 2020

zone117x commented Aug 12, 2020

yknl left a comment

kyranjamie Aug 18, 2020

kyranjamie Aug 18, 2020

reedrosenbluth Aug 19, 2020

yknl commented Aug 19, 2020

zone117x commented Aug 19, 2020 •

edited

zone117x commented Aug 20, 2020

zone117x commented Aug 21, 2020 •

edited

reedrosenbluth commented Aug 21, 2020

zone117x left a comment

		];

		const spendingConditionBytes = Buffer.from(spendingConditionBytesHex);

Multi Sig Transactions #109

Multi Sig Transactions #109

Conversation

reedrosenbluth commented Aug 5, 2020 • edited

Description

TODO

Type of Change

Does this introduce a breaking change?

Are documentation updates required?

Testing information

Checklist

codecov bot commented Aug 11, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reedrosenbluth Aug 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zone117x commented Aug 12, 2020

yknl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yknl commented Aug 19, 2020

zone117x commented Aug 19, 2020 • edited

zone117x commented Aug 20, 2020

zone117x commented Aug 21, 2020 • edited

reedrosenbluth commented Aug 21, 2020

zone117x left a comment

Choose a reason for hiding this comment

reedrosenbluth commented Aug 5, 2020 •

edited

codecov bot commented Aug 11, 2020 •

edited

reedrosenbluth Aug 12, 2020 •

edited

zone117x commented Aug 19, 2020 •

edited

zone117x commented Aug 21, 2020 •

edited