[wallet] Ensure wallet is unlocked before signing

gustavocoding commented at 5:16 pm on September 24, 2018: none

Fixes #14082

[wallet] Ensure wallet is unlocked before signing db15805668

in test/functional/rpc_signrawtransaction.py:51 in db15805668 outdated

44@@ -45,6 +45,14 @@ def successful_signing_test(self):
45         # 2) No script verification error occurred
46         assert 'errors' not in rawTxSigned
47 
48+    def test_with_lock_outputs(self):
49+        """Test correct error reporting when trying to sign a locked output"""
50+        self.nodes[0].encryptwallet("password")
51+        self.restart_node(0)

MarcoFalke commented at 6:48 pm on September 24, 2018:

Could add a second commit to remove the restart_node, which is not required on master, but on 0.17.

gustavocoding commented at 7:40 pm on September 24, 2018:

done

gustavocoding commented at 8:11 pm on September 24, 2018:

just need one more change, the signature method must be changed to be 0.16 compliant

gustavocoding commented at 8:14 pm on September 24, 2018:

now it’s properly backported

MarcoFalke added the label Wallet on Sep 24, 2018

MarcoFalke added the label RPC/REST/ZMQ on Sep 24, 2018

MarcoFalke added this to the milestone 0.17.0 on Sep 24, 2018

MarcoFalke added the label Needs backport on Sep 24, 2018

practicalswift commented at 6:55 pm on September 24, 2018: contributor

Concept ACK

Nice first-time contribution!

in test/functional/rpc_signrawtransaction.py:54 in 5563b49ac1 outdated

49+        """Test correct error reporting when trying to sign a locked output"""
50+        self.nodes[0].encryptwallet("password")
51+
52+        rawTx = '020000000156b958f78e3f24e0b2f4e4db1255426b0902027cb37e3ddadb52e37c3557dddb0000000000ffffffff01c0a6b929010000001600149a2ee8c77140a053f36018ac8124a6ececc1668a00000000'
53+
54+        assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].signrawtransaction, rawTx)

achow101 commented at 1:02 am on September 25, 2018:

signrawtransaction no longer exists in master. You should change this to signrawtransactionwithwallet. This will also fix travis.

gustavocoding commented at 6:02 am on September 25, 2018:

ok, fixed, thanks

[wallet] remove redundand restart node 20442f617f

MarcoFalke commented at 11:31 pm on September 25, 2018: member

utACK 20442f617f7f86cbdde1c41c1165e2b750f756c7 (also run the test on the 0.16 branch to check that it passes without any code changes)

achow101 commented at 0:09 am on September 26, 2018: member

utACK 20442f617f7f86cbdde1c41c1165e2b750f756c7

in test/functional/rpc_signrawtransaction.py:52 in 20442f617f

44@@ -45,6 +45,14 @@ def successful_signing_test(self):
45         # 2) No script verification error occurred
46         assert 'errors' not in rawTxSigned
47 
48+    def test_with_lock_outputs(self):
49+        """Test correct error reporting when trying to sign a locked output"""
50+        self.nodes[0].encryptwallet("password")
51+
52+        rawTx = '020000000156b958f78e3f24e0b2f4e4db1255426b0902027cb37e3ddadb52e37c3557dddb0000000000ffffffff01c0a6b929010000001600149a2ee8c77140a053f36018ac8124a6ececc1668a00000000'

promag commented at 0:26 am on September 26, 2018:

Could create a transaction that the wallet could actually sign?

Without this fix, this test holds:

0        self.nodes[0].generate(101)
1        res = self.nodes[0].createrawtransaction([], {'mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB': 0.1})
2        res = self.nodes[0].fundrawtransaction(res)
3        self.nodes[0].encryptwallet("password")
4        res = self.nodes[0].signrawtransactionwithwallet(res['hex'])
5        assert_equal(res['errors'][0]['error'], 'Unable to sign input, invalid stack size (possibly missing key)')

gustavocoding commented at 7:55 am on September 26, 2018:

Yes, this test passes on master and 0.17, but the point of this PR is reporting the correct error before signing, so it’s sufficient that the tx is well formed.

promag commented at 8:00 am on September 26, 2018:

Yes, just saying because I had to write that to test the change is necessary.

gustavocoding commented at 8:16 am on September 26, 2018:

Another way to verify the necessity of the change is to run this PR’s test_with_lock_outputs in the 0.16 branch, it will pass.

murtyjones commented at 1:48 am on September 26, 2018: contributor

ACK – tested on master, 0.17, 0.16

promag commented at 8:29 am on September 26, 2018: member

Tested ACK 20442f6.

MarcoFalke referenced this in commit d799efe214 on Sep 26, 2018

MarcoFalke merged this on Sep 26, 2018

MarcoFalke closed this on Sep 26, 2018

MarcoFalke referenced this in commit 5fb6f55bae on Sep 26, 2018

MarcoFalke removed the label Needs backport on Sep 26, 2018

gustavocoding deleted the branch on Oct 2, 2018

gustavocoding commented at 6:14 pm on October 3, 2018: none

Although milestone was set to 0.17, I can’t see the changes of this PR on 0.17.0 that was just released: https://github.com/bitcoin/bitcoin/blob/v0.17.0/test/functional/rpc_signrawtransaction.py

MarcoFalke removed this from the milestone 0.17.0 on Oct 3, 2018

MarcoFalke added this to the milestone 0.17.1 on Oct 3, 2018

MarcoFalke commented at 9:47 pm on October 3, 2018: member

Indeed, #14328 missed the 0.17.0 and will likely be merged before 0.17.1

MarcoFalke referenced this in commit 192894ef0a on Oct 28, 2018

MarcoFalke referenced this in commit bb90695551 on Nov 28, 2018

deadalnix referenced this in commit ddcbef8bf4 on Feb 12, 2020

pravblockc referenced this in commit c1a3eb6a68 on Jul 28, 2021

pravblockc referenced this in commit e68964ca20 on Jul 30, 2021

MarcoFalke locked this on Sep 8, 2021

[wallet] Ensure wallet is unlocked before signing #14310