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: For feebump, ignore abandoned descendant spends #26675

Merged
merged 1 commit into from
Jan 11, 2023
Merged

wallet: For feebump, ignore abandoned descendant spends #26675

merged 1 commit into from
Jan 11, 2023

Conversation

john-moffett
Copy link
Contributor

Closes #26667

To be eligible for fee-bumping, a transaction must not have any of its outputs (eg - change) spent in other unconfirmed transactions in the wallet. This behavior is currently enforced and tested.

However, this check shouldn't apply to spends in abandoned descendant transactions, as explained by #26667.

CWallet::IsSpent already carves out an exception for abandoned transactions, so we can just use that.

I've also added a new test to cover this case.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 9, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK Sjors, furszy, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Wallet label Dec 9, 2022
@fanquake fanquake requested a review from Sjors December 10, 2022 10:41
Sjors
Sjors approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

utACK 7c325c2

(I also checked that the test fails without the change in HasWalletSpend())

test/functional/wallet_bumpfee.py Outdated
change_output = next(o for o in output_list if o["value"] == Decimal("0.00049000"))

# Submit child transaction with low fee
child = rbf_node.createrawtransaction([{"txid": parent_id, "vout": change_output["n"]}], {dest_address: 0.0004850})
Copy link
Member

Choose a reason for hiding this comment

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

Might be easier to use the send RPC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change it, but I picked this because send is marked as "EXPERIMENTAL warning: this call may be changed in future releases". I do see other uses of it in tests, though.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to use it in tests. If we ever change the interface, we'll have to refactor the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I updated the test to use send and it did end up simplifying the test code a bit. Thanks!

@achow101
Copy link
Member

ACK 7c325c2

@john-moffett
For feebump, ignore abandoned descendant spends
f9ce0ea 
To be eligible for fee-bumping, a transaction must not have any
of its outputs (eg - change) spent in other unconfirmed transactions
in the wallet. However, this check should not apply to abandoned
transactions.

A new test case is added to cover this case.
@john-moffett john-moffett force-pushed the 2022_12_Bumpfee_ignore_abandoned branch from 7c325c2 to f9ce0ea Compare December 15, 2022 19:39
@Sjors
Copy link
Member

Sjors commented Dec 15, 2022

re-utACK f9ce0ea

furszy
furszy approved these changes Dec 29, 2022
View reviewed changes
Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK f9ce0ea

@achow101
Copy link
Member

ACK f9ce0ea

@achow101 achow101 merged commit fbe5e12 into bitcoin:master Jan 11, 2023
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 12, 2023
@john-moffett @fanquake
For feebump, ignore abandoned descendant spends
6a7b253 
To be eligible for fee-bumping, a transaction must not have any
of its outputs (eg - change) spent in other unconfirmed transactions
in the wallet. However, this check should not apply to abandoned
transactions.

A new test case is added to cover this case.

Github-Pull: bitcoin#26675
Rebased-From: f9ce0ea
@fanquake fanquake mentioned this pull request Jan 12, 2023
Merged
@fanquake
Copy link
Member

Backported in #26878.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 12, 2023
@john-moffett @fanquake
For feebump, ignore abandoned descendant spends
91f7021 
To be eligible for fee-bumping, a transaction must not have any
of its outputs (eg - change) spent in other unconfirmed transactions
in the wallet. However, this check should not apply to abandoned
transactions.

A new test case is added to cover this case.

Github-Pull: bitcoin#26675
Rebased-From: f9ce0ea
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 12, 2023
@achow101
Merge bitcoin#26675: wallet: For feebump, ignore abandoned descendant…
f979b16 
… spends

f9ce0ea For feebump, ignore abandoned descendant spends (John Moffett)

Pull request description:

  Closes bitcoin#26667

  To be eligible for fee-bumping, a transaction must not have any of its outputs (eg - change) spent in other unconfirmed transactions in the wallet. This behavior is currently [enforced](https://github.com/bitcoin/bitcoin/blob/9e229a542ff2107be43eff2e4b992841367f0366/src/wallet/feebumper.cpp#L25-L28) and [tested](https://github.com/bitcoin/bitcoin/blob/9e229a542ff2107be43eff2e4b992841367f0366/test/functional/wallet_bumpfee.py#L270-L286).

  However, this check shouldn't apply to spends in abandoned descendant transactions, as explained by bitcoin#26667.

  `CWallet::IsSpent` already carves out an exception for abandoned transactions, so we can just use that.

  I've also added a new test to cover this case.

ACKs for top commit:
  Sjors:
    re-utACK f9ce0ea
  achow101:
    ACK f9ce0ea
  furszy:
    ACK f9ce0ea

Tree-SHA512: 19d957d1cf6747668bb114e27a305027bfca5a9bed2b1d9cc9e1b0bd4666486c7c4b60b045a7fe677eb9734d746f5de76390781fb1e9e0bceb4a46d20acd1749
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 22, 2023
@john-moffett @fanquake
For feebump, ignore abandoned descendant spends
5c824ac 
To be eligible for fee-bumping, a transaction must not have any
of its outputs (eg - change) spent in other unconfirmed transactions
in the wallet. However, this check should not apply to abandoned
transactions.

A new test case is added to cover this case.

Github-Pull: bitcoin#26675
Rebased-From: f9ce0ea
glozow added a commit that referenced this pull request Feb 27, 2023
@glozow
Merge #26878: [24.x] Backports
c8c85ca 
784a754 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow)
debcfe3 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow)
ccc72fe wallet: Be able to unlock the wallet for migration (Andrew Chow)
50dd8b1 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow)
648b062 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow)
ab3bd45 i2p: use consistent number of tunnels with i2pd and Java I2P (Vasil Dimov)
29cdf42 i2p: lower the number of tunnels for transient sessions (Vasil Dimov)
5027e93 i2p: reuse created I2P sessions if not used (Vasil Dimov)
a62c541 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
64e7db6 Zero out wallet master key upon lock (John Moffett)
b7e242e Correctly limit overview transaction list (John Moffett)
cff6718 depends: fix systemtap download URL (fanquake)
7cf73df Add missing includes to fix gcc-13 compile error (MarcoFalke)
07397cd addrdb: Only call Serialize() once (Martin Zumsande)
91f83db hash: add HashedSourceWriter (Martin Zumsande)
5c824ac For feebump, ignore abandoned descendant spends (John Moffett)
428dcd5 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow)
cbcdafa test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner)
342abfb wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner)

Pull request description:

  Backports:
  * #26595
  * #26675
  * #26679
  * #26761
  * #26837
  * #26909
  * #26924
  * #26944
  * bitcoin-core/gui#704
  * #27053
  * #27080

ACKs for top commit:
  instagibbs:
    ACK 784a754
  achow101:
    ACK 784a754
  hebasto:
    ACK 784a754, I've made backporting locally and got a diff between my branch and this PR as follows:

Tree-SHA512: 8ea84aa02d7907ff1e202e1302b441ce9ed2198bf383620ad40056a5d7e8ea88e1047abef0b92d85648016bf9b3195c974be3806ccebd85bef4f85c326869e43
@bitcoin bitcoin locked and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Sjors Sjors Sjors approved these changes

@furszy furszy furszy approved these changes

@onyeali501 onyeali501 onyeali501 approved these changes

Assignees

@achow101 achow101

Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't bump fee after abandoning child transaction
7 participants
@john-moffett @DrahtBot @achow101 @Sjors @fanquake @furszy @onyeali501