btcd icon indicating copy to clipboard operation
btcd copied to clipboard

Nested witness signing

Open afk11 opened this issue 5 years ago • 8 comments

This PR adds SignTxWitness which handles signing normal outputs + any that use segwit. It will return a signature script and witness structure after signing whatever it can and merging signatures. The function is a superset of what SignTxOutput supports. I left SignTxOutput's intact to preserve BC, though SignTxWitness also works with old outputs and should probably be used going forward for easy segwit support.

I went with using [][]byte to pass around input script data because it's easily converted back to a pushonly scriptSig, or can be used as a witness if called for.

Also added test cases comparable to SignTxOutput:

  • P2WPKH: bare and P2SH
  • P2PKH: P2SH, P2WSH and P2SH|P2WSH
  • P2PK: P2SH, P2WSH and P2SH|P2WSH
  • Multisig 2of2 (basic, step by step, & with key duplication): P2SH, P2WSH and P2SH|P2WSH

afk11 avatar Sep 22 '18 12:09 afk11

@jcvernaleo (as per #1530)

  • Low priority
  • Enhancement
  • Outdated

jakesylvestre avatar Mar 04 '20 14:03 jakesylvestre

any progress on this?

siansong avatar Oct 26 '20 11:10 siansong

@sunsiansong It seems the PR was lost in the backlog. I've added this to our internal review board to prioritize this.

In the meantime, if you can help with reviewing and testing out this PR, that'd be very helpful!

onyb avatar Oct 26 '20 11:10 onyb

@afk11 If you could rebase your branch on the current master to run the CI checks and fix the outdated Glide stuff, that'd really get the ball rolling.

onyb avatar Oct 26 '20 11:10 onyb

Hi all

Thanks for the ping. I know most apps got by without this feature, but did think it was worthwhile to bring everything up to date. I'll try rebase this in the next day or so.

Re tests, I covered most nested forms of P2PK/P2PKH/multisig in P2SH, P2WSH, and nested P2WSH in P2SH with the results of each verified by the script interpreter. So I'm pretty confident it was working :) https://github.com/btcsuite/btcd/pull/1304/files#diff-c4fcde2c18ebe144b7b71b53efb1021c7bb089a8c02602134402999ca9fa1417R1436

I'll check in more depth later and report back

afk11 avatar Oct 27 '20 14:10 afk11

Pull Request Test Coverage Report for Build 343822214

  • 184 of 195 (94.36%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.2%) to 53.581%

Changes Missing Coverage Covered Lines Changed/Added Lines %
txscript/sign.go 184 195 94.36%
<!-- Total: 184 195
Files with Coverage Reduction New Missed Lines %
txscript/sign.go 1 90.6%
peer/peer.go 2 75.55%
connmgr/connmanager.go 3 86.07%
btcec/signature.go 6 91.64%
<!-- Total: 12
Totals Coverage Status
Change from base Build 329751959: 0.2%
Covered Lines: 20910
Relevant Lines: 39025

💛 - Coveralls

coveralls avatar Oct 27 '20 19:10 coveralls

Squashed @jcvernaleo

afk11 avatar Nov 03 '20 15:11 afk11

@jcvernaleo @onyb anything else you'd like done on this?

afk11 avatar Nov 09 '20 17:11 afk11