Skip to content

wallet: Fully process previous RPCs before accepting new ones #18840

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
Closed
maflcko wants to merge 1 commit into from
Closed

wallet: Fully process previous RPCs before accepting new ones #18840

maflcko wants to merge 1 commit into from

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 1, 2020

The wallet may process calls without waiting on the result and effects of previous calls. This causes failures in user scripts, because later RPCs may depend on the state changes from previous RPCs.

For example, the following might fail on current master:

txid = sendtoaddress(...)
bumpfee(txid)
abandontransaction(txid)  # fails because tx is still in the mempool

Fixes #18831

@maflcko maflcko changed the title wallet: Fully process previous wallet calls before accepting new ones wallet: Fully process previous RPCs before accepting new ones May 1, 2020
@fanquake fanquake added the Wallet label May 1, 2020
@ryanofsky
Copy link
Contributor

ryanofsky commented May 1, 2020

I think the description of this change is misleading. With this change the wallet isn't just waiting to "fuly process previous RPCs" it's also waiting for the node to process it's own internal events, and is waiting for other external listeners like the GUI, the transaction index, p2p networking code, and other wallets that are loaded.

I'm also not sure if the change reliably fixes the reported problem, or just fixes it by accidentally by adding a delay. How does SyncWithValidationInterfaceQueue() ensure the new transaction is added to the mempool? And could there be a more direct fix for the abandontransaction error like having bumpfee update the old transaction's fInMempool flag directly after the new transaction is added?

@maflcko
Copy link
Member Author

maflcko commented May 1, 2020

ok, closing then

@maflcko maflcko closed this May 1, 2020
@maflcko maflcko deleted the 2004-walletFullyProcessRPCs branch May 1, 2020 14:52
@maflcko
Copy link
Member Author

maflcko commented May 1, 2020

How does SyncWithValidationInterfaceQueue() ensure the new transaction is added to the mempool?

This is not about the new transaction, it is about the replaced transaction (txid in my example that fails). So SyncWithValidationInterfaceQueue ensures that the transaction is properly marked as removed from the mempool when it has in fact been removed.

And could there be a more direct fix for the abandontransaction error like having bumpfee update the old transaction's fInMempool flag directly after the new transaction is added?

Sure, see #18842. But keep in mind that #18842 is less correct, as a transaction that is still in the mempool and has been marked as removed will never be correctly marked as in mempool again.

@maflcko maflcko mentioned this pull request May 1, 2020
Merged
@kanzure
Copy link
Contributor

kanzure commented May 1, 2020

I think something like this pull request could help with some (but not all) RPC request isolation issues like #9197 (comment)

@maflcko maflcko mentioned this pull request Jun 29, 2021
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Intermittent failure in feature_backwards_compatibility "Transaction not eligible for abandonment"

4 participants

@maflcko @ryanofsky @kanzure @fanquake