'mandatory-script-verify-flag-failed (Signature must use SIGHASH_FORKID) #4
Comments
|
SendRequest.setUseForkId() will be needed. There is probably a better way to handle this. to maintain backward compatibility with bitcoinj unit tests, the default signature hash function is the Bitcoin version. But for Bitcoin Cash, it will be the replay protection signature hash. |
|
Should we remove this need for the 0.14 release?
Daniel
… -------- Original Message --------
Subject: Re: [bitcoinj-cash/bitcoinj] 'mandatory-script-verify-flag-failed (Signature must use SIGHASH_FORKID) (#4)
Local Time: December 13, 2017 9:05 AM
UTC Time: December 13, 2017 8:05 AM
From: ***@***.***
To: bitcoinj-cash/bitcoinj ***@***.***>
Subscribed ***@***.***>
SendRequest.setUseForkId() will be needed.
There is probably a better way to handle this. to maintain backward compatibility with bitcoinj unit tests, the default signature hash function is the Bitcoin version. But for Bitcoin Cash, it will be the replay protection signature hash.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, [view it on GitHub](#4 (comment)), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AB8Jz5yXFSnlyoaH65cUdrLrMwzAOVt5ks5s_4VegaJpZM4RAJGY).
|
|
It would be easier for users of the library if we removed the requirement. Somehow we still need the unit tests to work. There are a few unit tests that will probably fail, if we change the sighash to the BCH version. It may not be that many tests that would have to be modified to use the BTC sighash. |
|
I've got an initial version here: https://github.com/Danconnolly/bitcoinj-cash/tree/setforkid
I've added a Transaction constructor with a version parameter and then adjusted the default constructor to use that one with a default version of 2 (Bitcoin Cash). Updated failing tests to use a version 1 transaction (e.g. some of the tests re-create the genesis block using a transaction). The payment channel classes all had to be updated to use version 1 transactions, they won't work with BCH at the moment but I dont think anyone is using them. Added that to my to-do list.
Still to do: look at the SendRequest:setForkId() to make it fit (probably have it just do nothing, mark as deprecated, remove in future version).
All the tests pass (https://travis-ci.org/Danconnolly/bitcoinj-cash/builds/317104133) but I wanted to do so further checking.
Daniel
… -------- Original Message --------
Subject: Re: [bitcoinj-cash/bitcoinj] 'mandatory-script-verify-flag-failed (Signature must use SIGHASH_FORKID) (#4)
Local Time: December 18, 2017 8:05 AM
UTC Time: December 18, 2017 7:05 AM
From: ***@***.***
To: bitcoinj-cash/bitcoinj ***@***.***>
Daniel Connolly ***@***.***>, Comment ***@***.***>
It would be easier for users of the library if we removed the requirement. Somehow we still need the unit tests to work. There are a few unit tests that will probably fail, if we change the sighash to the BCH version. It may not be that many tests that would have to be modified to use the BTC sighash.
—
You are receiving this because you commented.
Reply to this email directly, [view it on GitHub](#4 (comment)), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AB8Jz6IaJ6lzSGGJsyAL_vq3rOM436YCks5tBg7BgaJpZM4RAJGY).
|
|
I'd appreciate some review, opinions, comments.
… -------- Original Message --------
Subject: Re: [bitcoinj-cash/bitcoinj] 'mandatory-script-verify-flag-failed (Signature must use SIGHASH_FORKID) (#4)
Local Time: December 18, 2017 9:00 AM
UTC Time: December 18, 2017 8:00 AM
From: ***@***.***
To: bitcoinj-cash/bitcoinj ***@***.***>
I've got an initial version here: https://github.com/Danconnolly/bitcoinj-cash/tree/setforkid
I've added a Transaction constructor with a version parameter and then adjusted the default constructor to use that one with a default version of 2 (Bitcoin Cash). Updated failing tests to use a version 1 transaction (e.g. some of the tests re-create the genesis block using a transaction). The payment channel classes all had to be updated to use version 1 transactions, they won't work with BCH at the moment but I dont think anyone is using them. Added that to my to-do list.
Still to do: look at the SendRequest:setForkId() to make it fit (probably have it just do nothing, mark as deprecated, remove in future version).
All the tests pass (https://travis-ci.org/Danconnolly/bitcoinj-cash/builds/317104133) but I wanted to do so further checking.
Daniel
> -------- Original Message --------
> Subject: Re: [bitcoinj-cash/bitcoinj] 'mandatory-script-verify-flag-failed (Signature must use SIGHASH_FORKID) (#4)
> Local Time: December 18, 2017 8:05 AM
> UTC Time: December 18, 2017 7:05 AM
> From: ***@***.***
> To: bitcoinj-cash/bitcoinj ***@***.***>
> Daniel Connolly ***@***.***>, Comment ***@***.***>
>
> It would be easier for users of the library if we removed the requirement. Somehow we still need the unit tests to work. There are a few unit tests that will probably fail, if we change the sighash to the BCH version. It may not be that many tests that would have to be modified to use the BTC sighash.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, [view it on GitHub](#4 (comment)), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AB8Jz6IaJ6lzSGGJsyAL_vq3rOM436YCks5tBg7BgaJpZM4RAJGY).
|
|
I think the provided solution is a good balance between backwards compatibility and introduced complexity. Regards, |
|
So this has been quiet for a while. I am having issues where the network is rejecting my tx that were signed offline using bitcoinj.cash, exactly because of the reasons above. I've tried creating the transaction (from unsigned hex tx) and setting Any solutions yet? |
|
@robinkanters For now you will need to use @Danconnolly Your suggested changes above are a good start, but we need make changes to SendRequest and Wallet to |
|
@HashEngineering I don't have a SendRequest, only a Transaction (offline signing) |
|
@robinkanters In that case, we will need to make a different change. How do you to sign your transaction? |
This works for BTC, but BCH rejects the tx for the fork id |
|
I think you need to use Btw @HashEngineering since this is the most common case I think this should be done by default. |
|
I think I tried that, will try again tomorrow and see if I can post some code. It's in Kotlin but should be understandable enough ;) |
|
I can read Kotlin. The issue is that the tx is signed using BTC methods,
instead of BCH methods. Signing in bitcoinj-cash needs needs some changes
to use the correct signing method. Specifying Version 2 alone does not
currently allow for that.
…On Feb 13, 2018 2:20 PM, "Robin Kanters" ***@***.***> wrote:
I think I tried that, will try again tomorrow and see if I can post some
code. It's in Kotlin but should be understandable enough ;)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFriIfDWZoGyQU_2FT40hCUtpkZh6edCks5tUgqagaJpZM4RAJGY>
.
|
|
@rseibane SendRequest.useForkId(true) does set the transaction version to 2, but it also triggers bitcoinj to use the Bitcoin Cash signature creation. Having a transaction version 2 doesn't trigger anything, currently. However, it should, as you say, be the default behavior (tx version 2 and sign with forkId). |
|
@HashEngineering did not mean to be condescending, just not used to dealing with people familiar with Kotlin ;) Anyway, this is the code I'm currently using to sign raw incoming txs. Don't know if this is correct, but to my understanding this should work val txHex = Hex.decode(transactionRaw)
val t = Transaction(params, txHex)
t.confidence.source = TransactionConfidence.Source.SELF
t.purpose = Transaction.Purpose.USER_PAYMENT
val inputs = t.inputs.toList()
t.clearInputs()
inputs.forEach {
val key: ECKey = // getting and parsing ECKey from db
t.addSignedInput(it.outpoint, it.scriptSig, key)
}
return String(Hex.encode(t.bitcoinSerialize())) |
|
This: should be: It looks like |
|
@HashEngineering thanks, unfortunately that doesn't work either :( Your code misses the SigHash parameter anyway, I made it into I'm thinking the library really does need changes for this to work, like you said. Just in case I'm doing something stupid, this is my (signed) tx: and this is what I got before your comment: Both are rejected by network rules for missing a fork id. For completeness, this is my unsigned tx: |
|
I will need to look at this later. My comment did result in a different signature than yours, so some part of it "works". It's possible that having a version 2 transaction might help, but I doubt it. |
|
I can try it, at least. Do I just call some Edit: v2 tx here, also rejected: |
|
I started to make a test based on your code above and then I found the problem and it has to do with making the transaction "offline!" Your signature is not correct due to missing information. This is the code that does the work -- public TransactionInput addSignedInput(TransactionOutPoint prevOut, Script scriptPubKey, ECKey sigKey,
SigHash sigHash, boolean anyoneCanPay, boolean forkId) throws ScriptException {
// Verify the API user didn't try to do operations out of order.
checkState(!outputs.isEmpty(), "Attempting to sign tx without outputs.");
TransactionInput input = new TransactionInput(params, this, new byte[]{}, prevOut);
addInput(input);
Sha256Hash hash = forkId ?
hashForSignatureWitness(inputs.size() -1, scriptPubKey, prevOut.getConnectedOutput().getValue(), sigHash, anyoneCanPay) :
hashForSignature(inputs.size() - 1, scriptPubKey, sigHash, anyoneCanPay);
ECKey.ECDSASignature ecSig = sigKey.sign(hash);
TransactionSignature txSig = new TransactionSignature(ecSig, sigHash, anyoneCanPay, forkId);
if (scriptPubKey.isSentToRawPubKey())
input.setScriptSig(ScriptBuilder.createInputScript(txSig));
else if (scriptPubKey.isSentToAddress())
input.setScriptSig(ScriptBuilder.createInputScript(txSig, sigKey));
else
throw new ScriptException("Don't know how to sign for this kind of scriptPubKey: " + scriptPubKey);
return input;
}The key line is this: If the forkId parameter is set true, then we use a different Signature Hash. One of the necessary pieces of information is the value of the input(prevOut.getConnectedOutput().getValue()) being signed. This information will not be available in a created transaction, whether in hex format or not. bitcoinj-cash is typically used to manage a wallet and when BCH are being spent, the wallet has the transaction information for the inputs, so it has the values of of those inputs as well. Therefore it can properly sign a new transaction. My Bitcoin Cash Wallet app is able to sweep a paper wallet where it queries an electron cash server to get the unspent outputs. From there it creates a wallet with those transactions and it is able to sign the sweep transaction properly. An upsent API at a block explorer can also be a source for such information. In your case, you will need to get the transaction information for the inputs and then for the inputs call |
|
@HashEngineering thanks for all the info. Yes, it does make sense, but I can't help but wonder if it really needs to be this way. I've developed virtually the same code for BTC but that works just fine. Seems to me that it should be possible for BCH as well. If not, I can always alter my API to accept the connected output value of course, but I don't particularly like that approach. |
|
@HashEngineering would it be possible to not clear the inputs but instead add the signatures afterwards? that seems to be what |
|
@robinkanters - I don't think there is a way around this issue by following what works for BTC. The whole point of changing the Signature Hash for BCH was for replay projection. BCH transactions are not valid on the BTC blockchain and vise versa. https://github.com/bitcoincashorg/spec/blob/master/replay-protected-sighash.md |
|
Yes I understand that the steps won't be completely the same, but I can't imagine it would now be impossible to sign txs offline |
|
Whether offline or online, knowledge of the value of the inputs should be known to properly calculate the outputs and fees. If you like, we could simplify the process for you and users doing similar work, by adding an amount parameter. This can eliminate connecting inputs to other transactions: |
|
@HashEngineering that would already be a big help! Still not ideal of course because I have to have a different API from BTC now, but already a lot better! I was just quickly browsing the code to see if this is something that I could create a PR for, but I don't think so... |
|
If your software is ever to support BTC segwit transactions, you will need similar modifications. The BCH signature hash method is the same as for BTC segwit transactions. When working with different coins, eventually one comes across the differences. |
|
@HashEngineering fair enough. I don't ever like asking this question on OSS projects but can you give me a very rough estimate on when this fix could be available? I need it for my job and would like to plan my next issues |
|
By the end of March, this should be done. This is one of several parts that need to be addressed with signatures. |
|
@robinkanters - if you get a chance, please take a look at Pull Request #28 and see if the additional |
|
@HashEngineering will check it out tomorrow, will report back |
|
@HashEngineering unfortunately I couldn't even hook it up to that branch because I'm still dependent on |
|
There are ways to get the code with git, but it gets complicated with so many branches from different repos. What you can do from your branch where you are working with If this works, then it will change the code on a new branch, so it doesn't mess up what you currently have. It may give an error. After you determine if one of the new If using git doesn't work, it would require looking at the code changes and doing it yourself for the one or two methods that you needed changed. |
|
okay disregard what I posted earlier. Just successfully signed and sent tx t.addSignedInput(transactionOutPoint, it.scriptSig, key, Transaction.SigHash.ALL, true, true) |
|
@robinkanters, do you need any addSignedInput methods with a |
|
@HashEngineering not needed by me, the one I'm currently using is enough for my use case |
I have a problem:
org.bitcoinj.core.RejectedTransactionException: Reject: tx for reason 'mandatory-script-verify-flag-failed (Signature must use SIGHASH_FORKID)' (16)
at org.bitcoinj.core.TransactionBroadcast$2.onPreMessageReceived(TransactionBroadcast.java:102) [bitcoincashj-core-0.14.5-SNAPSHOT.jar:?]
at org.bitcoinj.core.Peer.processMessage(Peer.java:461) [bitcoincashj-core-0.14.5-SNAPSHOT.jar:?]
at org.bitcoinj.core.PeerSocketHandler.receiveBytes(PeerSocketHandler.java:182) [bitcoincashj-core-0.14.5-SNAPSHOT.jar:?]
at org.bitcoinj.net.ConnectionHandler.handleKey(ConnectionHandler.java:223) [bitcoincashj-core-0.14.5-SNAPSHOT.jar:?]
at org.bitcoinj.net.NioClientManager.handleKey(NioClientManager.java:86) [bitcoincashj-core-0.14.5-SNAPSHOT.jar:?]
at org.bitcoinj.net.NioClientManager.run(NioClientManager.java:122) [bitcoincashj-core-0.14.5-SNAPSHOT.jar:?]
at com.google.common.util.concurrent.AbstractExecutionThreadService$1$2.run(AbstractExecutionThreadService.java:60) [guava-18.0.jar:?]
at com.google.common.util.concurrent.Callables$3.run(Callables.java:95) [guava-18.0.jar:?]
at org.bitcoinj.utils.ContextPropagatingThreadFactory$1.run(ContextPropagatingThreadFactory.java:49) [bitcoincashj-core-0.14.5-SNAPSHOT.jar:?]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_73]
2017-12-13 07:40:24 ERROR Peer [514]-[119.23.160.146]:8333 /Bitcoin ABC:0.16.1(EB8.0)/: Received Reject
The text was updated successfully, but these errors were encountered: