Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

fix bip66 issue with signatures #167

Merged
merged 1 commit into from
Jul 30, 2015
Merged

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented Jul 29, 2015

Tested 10000 transactions on regtest with old version: got 44/10000 failures; see http://pastebin.com/u6imE6MC for txhex of each failed tx. A quick way to find the source of the error is to search for the string '021f' (although it could be lower than 1f, but the likelihood of that is near zero).

Tested 10000 with this bip66 correction: got 0/10000 failures (of any type).

The value 44/10000 is closely in line with expectations: 1/(256*2) is the expected probability of this failure mode for one DER encoded value (half of the cases where the first byte of 32 is zero). However, each of these transactions had 1 input, meaning 2 DER encoded values, leading to a 1/256 or ~40/10000 expected number of failures.

Note that all failures were associated with the same error string: "error: {"code":-26,"message":"64: non-mandatory-script-verify-flag (No error)" , as expected.

Since a real joinmarket transaction might have ~6 inputs, we could well see 1/20 to 1/50 transactions failing this way.

It may be worth updating to https://github.com/simcity4242/pybitcointools/ , although I know we are considering using a different codebase entirely. simcity4242 seems to have identified a few other issues, although I can't say how important they are.

@chris-belcher
Copy link
Collaborator

Good work!

After this gets merged I'll send an alert asking yield generators to update.

And look a bit at the other stuff in simcity4242's repos

chris-belcher added a commit that referenced this pull request Jul 30, 2015
fix bip66 issue with signatures
@chris-belcher chris-belcher merged commit 94ae340 into JoinMarket-Org:master Jul 30, 2015
@benma
Copy link

benma commented Aug 6, 2015

This patch was applied to the bitcoin subdirectory, which is a copy of pybitcointools (https://github.com/vbuterin/pybitcointools/). This patch should be applied upstream. The bitcoin subdir in joinmarket should ideally only be a git submodule to pybitcointools or a joinmarket fork of it, so diverging code can be tracked.

@AdamISZ
Copy link
Member Author

AdamISZ commented Aug 6, 2015

@benma yes, but the issue is that fix the was applied by user @simcity4242, who created his own fork because the repo owner @vbuterin is not responding to PRs (at least it seems that way). See the notes above re: @simcity4242 's fork, and how this interacts with the other issue here about moving away from pybitcointools.

@AdamISZ AdamISZ deleted the bip66 branch August 6, 2015 10:10
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Oct 1, 2015
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Oct 4, 2015
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Dec 4, 2015
fix bip66 issue with signatures
[gitreformat yapf-ify (github/ghtdak) on Fri Dec  4 04:50:29 2015]
[from commit: 94ae340]
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants