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

Fix signing raw transactions with unsynced offline nodes #3520

Merged
merged 4 commits into from Sep 19, 2018

Conversation

@Eirik0
Copy link
Contributor

Eirik0 commented Sep 14, 2018

This PR address the issue in two different ways:

  • In signrawtransaction we determine the consensus branch ID (which we then later use to construct the transaction) using the chain height. We now also consider the APPROX_RELEASE_HEIGHT as this is a better estimation than 0 for the height of the chain if we are unsynced. (This in and of itself solves the Overwinter signing issue).
  • We have added an additional parameter to signrawtransaction to allow manually overriding the consensus branch ID that zcashd determines we are on. This allows users to work around corner cases where the first strategy is still insufficient.

Closes #3327.

@Eirik0 Eirik0 self-assigned this Sep 14, 2018

@Eirik0 Eirik0 added this to In Review in Zcashd Team Sep 14, 2018

@str4d
Copy link
Contributor

str4d left a comment

Overall looks good. Some minor changes needed.

print self.nodes[0].sendrawtransaction(signed_hex)

# If we return the transaction hash, then we have have not thrown an error (success)
assert_true(len(self.nodes[0].sendrawtransaction(signed_tx['hex'])) > 0)

This comment has been minimized.

@str4d

str4d Sep 17, 2018

Contributor

Refactor this change into the previous commit, so that the test was originally failing.

Also, if you call offline_node.sendrawtransaction(...) first, then you should be able to assert that nodes[0].sendrawtransaction(...) returns the same result.

This comment has been minimized.

@Eirik0

Eirik0 Sep 17, 2018

Contributor

I think that it was failing when we tried to print below, but I will do this to make it more clear.

This comment has been minimized.

@Eirik0

Eirik0 Sep 17, 2018

Contributor

Calling offline_node.sendrawtransaction(...) fails because JSONRPC error: 16: tx-overwinter-not-active

@@ -92,6 +92,8 @@ BOOST_AUTO_TEST_CASE(rpc_rawparams)
BOOST_CHECK_NO_THROW(CallRPC(string("signrawtransaction ")+rawtx+" null null NONE|ANYONECANPAY"));
BOOST_CHECK_NO_THROW(CallRPC(string("signrawtransaction ")+rawtx+" [] [] NONE|ANYONECANPAY"));
BOOST_CHECK_THROW(CallRPC(string("signrawtransaction ")+rawtx+" null null badenum"), runtime_error);
BOOST_CHECK_NO_THROW(CallRPC(string("signrawtransaction ")+rawtx+" [] [] NONE|ANYONECANPAY 1537743641"));

This comment has been minimized.

@str4d

str4d Sep 17, 2018

Contributor

Use the hex representation of this number, so it visibly matches a consensus branch ID.

This comment has been minimized.

@Eirik0

Eirik0 Sep 17, 2018

Contributor

Redid this commit to use a hex string rather than int.

@str4d str4d added this to the v2.0.1 milestone Sep 17, 2018

@Eirik0 Eirik0 force-pushed the Eirik0:3327-sign-offline branch 2 times, most recently from e218ee6 to e32586b Sep 17, 2018

@Eirik0 Eirik0 force-pushed the Eirik0:3327-sign-offline branch from e32586b to 36a4906 Sep 17, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Sep 17, 2018

Addressed comments and force pushed

@str4d

str4d approved these changes Sep 18, 2018

@ebfull

ebfull approved these changes Sep 19, 2018

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Sep 19, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

📌 Commit 36a4906 has been approved by ebfull

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

⌛️ Testing commit 36a4906 with merge a69ec3d...

zkbot added a commit that referenced this pull request Sep 19, 2018

Auto merge of #3520 - Eirik0:3327-sign-offline, r=ebfull
Fix signing raw transactions with unsynced offline nodes

This PR address the issue in two different ways:

- In `signrawtransaction` we determine the consensus branch ID (which we then later use to construct the transaction) using the chain height. We now also consider the `APPROX_RELEASE_HEIGHT` as this is a better estimation than 0 for the height of the chain if we are unsynced. (This in and of itself solves the Overwinter signing issue).
- We have added an additional parameter to `signrawtransaction` to allow manually overriding the consensus branch ID that zcashd determines we are on. This allows users to work around corner cases where the first strategy is still insufficient.

Closes #3327.

@bitcartel bitcartel moved this from In Review to Released (Merged in Master) in Zcashd Team Sep 19, 2018

@bitcartel bitcartel moved this from Released (Merged in Master) to Complete in Zcashd Team Sep 19, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

💔 Test failed - pr-merge

@str4d str4d moved this from Complete to In Review in Zcashd Team Sep 19, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 19, 2018

Failed due to a pyflakes warning

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Sep 19, 2018

Should be fixed now.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 19, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

📌 Commit c10249f has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

⌛️ Testing commit c10249f with merge 36243f4...

zkbot added a commit that referenced this pull request Sep 19, 2018

Auto merge of #3520 - Eirik0:3327-sign-offline, r=bitcartel
Fix signing raw transactions with unsynced offline nodes

This PR address the issue in two different ways:

- In `signrawtransaction` we determine the consensus branch ID (which we then later use to construct the transaction) using the chain height. We now also consider the `APPROX_RELEASE_HEIGHT` as this is a better estimation than 0 for the height of the chain if we are unsynced. (This in and of itself solves the Overwinter signing issue).
- We have added an additional parameter to `signrawtransaction` to allow manually overriding the consensus branch ID that zcashd determines we are on. This allows users to work around corner cases where the first strategy is still insufficient.

Closes #3327.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 36243f4 to master...

@zkbot zkbot merged commit c10249f into zcash:master Sep 19, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment