Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
May 1, 2017
Merged

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

merged 1 commit into from
May 1, 2017

Conversation

instagibbs
Copy link
Member

@instagibbs instagibbs commented Apr 28, 2017 •
edited
Loading

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 #10293

@jonasschnelli
Copy link
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.

@instagibbs
Copy link
Member Author

@jonasschnelli yes I plan on writing one soon

@instagibbs
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

@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
Copy link
Member Author

instagibbs commented Apr 30, 2017 •
edited
Loading

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.

kallewoof
kallewoof reviewed May 1, 2017
View reviewed changes
Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fdd5d0eba1ecf7413d8b73ffb31a4ff82968f2ba with some nits.

test/functional/fundrawtransaction.py Outdated
@@ -53,6 +53,11 @@ def run_test(self):
self.nodes[0].generate(121)
self.sync_all()

# ensure that setting changPosition in fundraw with an exact match is handled properly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: changPosition missing an e

test/functional/fundrawtransaction.py Outdated
# ensure that setting changPosition in fundraw with an exact match is handled properly
rawmatch = self.nodes[2].createrawtransaction([], {self.nodes[2].getnewaddress():50})
rawmatch = self.nodes[2].fundrawtransaction(rawmatch, {"changePosition":1, "subtractFeeFromOutputs":[0]})
assert_equal(rawmatch["changepos"], -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@instagibbs instagibbs May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/wallet/wallet.cpp Outdated
@@ -2564,8 +2564,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
txNew.vout.insert(position, newTxOut);
}
}
else
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@instagibbs
Copy link
Member Author

fixed μ-nits

@kallewoof
Copy link
Contributor

utACK 7c58863

@laanwj
Copy link
Member

laanwj commented May 1, 2017

utACK 7c58863

@laanwj laanwj merged commit 7c58863 into bitcoin:master May 1, 2017
laanwj added a commit that referenced this pull request May 1, 2017
@laanwj
Merge #10294: [Wallet] unset change position when there is no change
e2b99b1 
7c58863 [Wallet] unset change position when there is no change on exact match (Gregory Sanders)

Tree-SHA512: ce8b9337e4132e32d80f954258d50938052c833a48e39431649d6adb16e3d18626a0ae5d300827e7fa397927fba72a1f066cb31af9b0a3ef7f1feb6024461626
@jonasschnelli jonasschnelli mentioned this pull request May 31, 2017
Merged
codablock pushed a commit to codablock/dash that referenced this pull request Jan 26, 2018
@laanwj @codablock
Merge bitcoin#10294: [Wallet] unset change position when there is no …
c45c767 
…change

7c58863 [Wallet] unset change position when there is no change on exact match (Gregory Sanders)

Tree-SHA512: ce8b9337e4132e32d80f954258d50938052c833a48e39431649d6adb16e3d18626a0ae5d300827e7fa397927fba72a1f066cb31af9b0a3ef7f1feb6024461626
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
@laanwj @andvgal
Merge bitcoin#10294: [Wallet] unset change position when there is no …
95e9ba6 
…change

7c58863 [Wallet] unset change position when there is no change on exact match (Gregory Sanders)

Tree-SHA512: ce8b9337e4132e32d80f954258d50938052c833a48e39431649d6adb16e3d18626a0ae5d300827e7fa397927fba72a1f066cb31af9b0a3ef7f1feb6024461626
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
@laanwj @CryptoCentric
Merge bitcoin#10294: [Wallet] unset change position when there is no …
496a399 
…change

7c58863 [Wallet] unset change position when there is no change on exact match (Gregory Sanders)

Tree-SHA512: ce8b9337e4132e32d80f954258d50938052c833a48e39431649d6adb16e3d18626a0ae5d300827e7fa397927fba72a1f066cb31af9b0a3ef7f1feb6024461626
@random-zebra random-zebra mentioned this pull request Apr 27, 2021
Merged
1 task
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Jun 13, 2021
@furszy
Merge #2341: [Wallet][RPC] Add subtract-fee-from-amount option in Cre…
e8d2cf0 
…ate[Fund]Transaction

a1bd590 [Doc] Document subtract-fee-from-amount RPC changes in the release notes (random-zebra)
6589429 wallet: first step generalizing CRecipient and ShieldedRecipient usage creating CRecipientBase class. (furszy)
1239506 [QA] Properly test change position in fundraw with sffa (random-zebra)
cc9ff09 [RPC] Add fSubtractFeeFromAmount to shieldsendmany (random-zebra)
65d3a80 [Refactoring] Subtract fee from destination when building shield tx (random-zebra)
cba898b [Refactoring] SendManyRecipient: use CRecipient for taddr + add sffa (random-zebra)
6fff9f5 [RPC] Add fSubtractFeeFromAmount to sendmany (transparent) (random-zebra)
7e30e3f [MOVE-ONLY] Move fundrawtransaction to rpcwallet (random-zebra)
499d62a [Tests] Add functional test for sffa in fundrawtransaction (random-zebra)
523b24d Subtract fee from amount (random-zebra)

Pull request description:

  Add a feature requested many times (#894, #1079, #2196, ...)

  Allow the user to deduct the fee from one or more of the transaction recipient amounts.
  The most common use-case is when sending the whole balance, or a selection of UTXOs, without getting any change back.
  This is already possible by selecting "after-fee-amount" in coin-control, but such metod is not user-friendly (e.g. in case of shield recipients, to get the correct value, the destinations must already be filled before opening coin-control and copying the "after-fee" value).
  Plus, this workaround is only available in the GUI.

  The subtract-fee option makes it easier, and enables it via the cli as well.

  Here it's exposed only to the RPC, both for transparent and shield recipients:
  - `sendtoaddress`
  - `sendmany`
  - `shieldsendmany`
  - `fundrawtransaction`

  GUI connection will be added in a successive PR (which will close those issues).

  Last commit is coming from bitcoin#10294

  Built on top of:
  - [x] #2337

ACKs for top commit:
  furszy:
    rebase + cherry-pick utACK a1bd590.
  Fuzzbawls:
    ACK a1bd590

Tree-SHA512: 7ed78f6ab8f1e9300d1468242e66016a4e1867f413256de23b7a60e103c2b628cdadcfc7998e8a7b252a7c2817d45261be108fad0eaa34d5ff892052c09b6e60
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kallewoof kallewoof kallewoof left review comments

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SEGV in segwit fundrawtransaction
4 participants
@instagibbs @jonasschnelli @kallewoof @laanwj