[Wallet] unset change position when there is no change #10294

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:fixchangepos changing 2 files +8 −2

instagibbs commented at 9:26 PM on April 28, 2017: member

Currently it remains set when there is an exact value match, and in the case of fundrawtransaction with a set value, it causes segfault when it attempts to insert it into the final CTransaction.

Fixes https://github.com/bitcoin/bitcoin/issues/10293
jonasschnelli commented at 9:28 PM on April 28, 2017: contributor

Thanks for the fix. utACK f0281be125896b85fad98d3ea907ccd2824f31a2

We may want to add a test for this. Shouldn't be too complicated, just fund a tx where amount = getbalance()-somefee.
jonasschnelli added the label Needs backport on Apr 28, 2017
jonasschnelli added the label Wallet on Apr 28, 2017
instagibbs commented at 9:29 PM on April 28, 2017: member

@jonasschnelli yes I plan on writing one soon
instagibbs force-pushed on Apr 30, 2017
instagibbs commented at 7:45 PM on April 30, 2017: member

Put in a test that caused segfault consistently otherwise.

slightly off-topic, I also ran into some weirdly created outputs via fundraw in master branch but can't seem to recreate anymore { "value": 0.00000101, "n": 1, "scriptPubKey": { "asm": "0 0 0 0", "hex": "00000000", "type": "nonstandard" } }
jonasschnelli commented at 8:00 PM on April 30, 2017: contributor

Thanks for the test. utACK 83abe86358aeae417f3d8b810e26ec12072f6526

slightly off-topic, I also ran into some weirdly created outputs via fundraw in master branch but can't seem to recreate anymore { "value": 0.00000101, "n": 1, "scriptPubKey": { "asm": "0 0 0 0", "hex": "00000000", "type": "nonstandard" } }

fundraw does only create one output, right (the change)? Or did it corrupted an existing output?
instagibbs commented at 8:03 PM on April 30, 2017: member

@jonasschnelli it indicated that that output was the change output, which is bizarre because it's a dust value and nonstandard, right? Perhaps it's the same bug, manifested in a different way?
instagibbs commented at 8:07 PM on April 30, 2017: member

Ok yes I am able to recreate it again on master but not on this branch. Must be the same bug. The rpc test I added will catch this case if it occurs.
instagibbs force-pushed on Apr 30, 2017

in test/functional/fundrawtransaction.py:56 in fdd5d0eba1 outdated

  52 | @@ -53,6 +53,11 @@ def run_test(self):
  53 |          self.nodes[0].generate(121)
  54 |          self.sync_all()
  55 |  
  56 | +        # ensure that setting changPosition in fundraw with an exact match is handled properly

kallewoof commented at 7:51 AM on May 1, 2017:

Typo: changPosition missing an e

in test/functional/fundrawtransaction.py:59 in fdd5d0eba1 outdated

  52 | @@ -53,6 +53,11 @@ def run_test(self):
  53 |          self.nodes[0].generate(121)
  54 |          self.sync_all()
  55 |  
  56 | +        # ensure that setting changPosition in fundraw with an exact match is handled properly
  57 | +        rawmatch = self.nodes[2].createrawtransaction([], {self.nodes[2].getnewaddress():50})
  58 | +        rawmatch = self.nodes[2].fundrawtransaction(rawmatch, {"changePosition":1, "subtractFeeFromOutputs":[0]})
  59 | +        assert_equal(rawmatch["changepos"], -1)

kallewoof commented at 7:55 AM on May 1, 2017:

This surprises me. I would assume changepos would equal changePosition that you supplied.

instagibbs commented at 12:04 PM on May 1, 2017:

if no resulting change exists, it will be -1 regardless. It was just broken before in the exact match case, sometimes returning a non-(-1) number with a garbage output, or segfault.

in src/wallet/wallet.cpp:2567 in fdd5d0eba1 outdated

2563 | @@ -2564,8 +2564,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
2564 |                          txNew.vout.insert(position, newTxOut);
2565 |                      }
2566 |                  }
2567 | -                else
2568 | +                else {

kallewoof commented at 7:55 AM on May 1, 2017:

Weak nit: Would love if this was } else { personally (i.e. no newline after }).

kallewoof commented at 7:56 AM on May 1, 2017: member

utACK fdd5d0eba1ecf7413d8b73ffb31a4ff82968f2ba with some nits.
[Wallet] unset change position when there is no change on exact match 7c588637d4
instagibbs force-pushed on May 1, 2017
instagibbs commented at 12:08 PM on May 1, 2017: member

fixed μ-nits
kallewoof commented at 12:16 PM on May 1, 2017: member

utACK 7c588637d467bfa18f48a7ca71c0700f90833ea1
laanwj commented at 1:18 PM on May 1, 2017: member

utACK https://github.com/bitcoin/bitcoin/commit/7c588637d467bfa18f48a7ca71c0700f90833ea1
laanwj merged this on May 1, 2017
laanwj closed this on May 1, 2017
laanwj referenced this in commit e2b99b1313 on May 1, 2017
luke-jr referenced this in commit 1d03baa891 on Jun 3, 2017
laanwj removed the label Needs backport on Sep 5, 2017
codablock referenced this in commit c45c767f8e on Jan 26, 2018
andvgal referenced this in commit 95e9ba6970 on Jan 6, 2019
CryptoCentric referenced this in commit 496a399101 on Feb 27, 2019
furszy referenced this in commit e8d2cf02e5 on Jun 13, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10293 SEGV in segwit fundrawtransaction #10484 0.14 Backports