Page MenuHomePhabricator

Use callbacks to cache whether wallet transactions are in mempool
ClosedPublic

Authored by deadalnix on Jan 8 2019, 02:02.

Details

Summary

This avoid calling out to mempool state during coin selection,
balance calculation, etc. In the next commit we ensure all wallet
callbacks from CValidationInterface happen in the same queue,
serialized with each other. This helps to avoid re-introducing one
of the issues described in #9584 [1] by further disconnecting
wallet from current chain/mempool state.

Thanks to @morcos for the suggestion to do this.

Note that there are several race conditions introduced here:

  • If a user calls sendrawtransaction from RPC, adding a transaction which is "trusted" (ie from them) and pays them change, it may not be immediately used by coin selection until the notification callbacks finish running. No such race is introduced in normal transaction-sending RPCs as this case is explicitly handled.
  • Until Block{Connected,Disconnected} and TransactionAddedToMempool calls also run in the CSceduler background thread, there is a race where TransactionAddedToMempool might be called after a Block{Connected,Disconnected} call happens.
  • Wallet will write a new best chain from the SetBestChain callback prior to having processed the transaction from that block.

[1] "you could go to select coins, need to use 0-conf change, but
such 0-conf change may have been included in a block who's
callbacks have not yet been processed - resulting in thinking they
are not in mempool and, thus, not selectable."

This is extracted from Core's PR10286 commit 17220d6325ef8d7789373055586e4332977077b0

Depends on D2268

Test Plan
make check
./test/functional/test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Jan 8 2019, 02:02
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 8 2019, 02:02
Herald added a subscriber: schancel. · View Herald Transcript
Fabien requested changes to this revision.Jan 8 2019, 12:59
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/wallet/wallet.cpp
4317 ↗(On Diff #6549)

bypass_limits => fOverrideMempoolLimit

This revision now requires changes to proceed.Jan 8 2019, 12:59
deadalnix updated this revision to Diff 6556.Jan 8 2019, 15:43

Fix naming in comments

Fabien accepted this revision.Jan 14 2019, 14:06
This revision is now accepted and ready to land.Jan 14 2019, 14:06
Closed by commit rABCad6b32a3e039: Use callbacks to cache whether wallet transactions are in mempool (authored by Matt Corallo <git@bluematt.me>, committed by deadalnix). · Explain WhyJan 14 2019, 14:59
This revision was automatically updated to reflect the committed changes.