Move fee estimator into validationinterface/cscheduler thread #11775

TheBlueMatt · 2017-11-27T19:30:34Z

This does a few things in a PR, so I'm happy to split it up if people prefer.

a) It clarifies the validationinterface stuff into a mempoolinterface and the validationinterface, splitting the two up into separate systems with the mempool creating events for mempoolinterface and validationinterface functions coming from validation. They are conjoined in the backend to provide ordering guarantees for listeners which are both mempool interfaces and validation interfaces, but are otherwise fully separated. There are a few cleanups that are enabled here, which I went ahead and did.

b) fee estimator becomes a listener to the mempool interface which provides a bit of additional info about transactions being added more than just the tx (eg feerate, etc)

c) a few minor edge cases in the fee estimator are handled (witness malleation changing feerate, and handling reorgs better, specifically).

TheBlueMatt · 2017-11-27T20:29:59Z

Obviously this cleans up some of the interface gook from #10286, and adds some more docs that probably could have been there. Next up after this one is moving alll the remaining events all into the background thread, with associated net_processing cleanups in the process.

TheBlueMatt · 2017-12-13T22:34:05Z

Rebased.

theuni · 2017-12-19T00:13:16Z

Concept ACK.

Though I'm nervous about backgrounding more stuff before safeguarding against outside (unsynchronized) access. For example, the mempool/feeestimator may now be significantly ahead of what the wallet has processed. So calling CreateTransaction when we're in the middle of connecting 10 blocks seems... dangerous, even with the BlockUntilSyncedToCurrentChain hack.

TheBlueMatt · 2017-12-19T00:34:39Z

Hmm? If anything the fee estimator should be at much lower risk there than for wallet...at worst you'll get out of date fee estimates...something you'd get anyway because you're calling the fee estimator in the middle of syncing stuff. Note that #11824 also keeps the queue depth at no more than 10, though I dont really want to rely on that and would prefer to implement the dump/reload-from-disk proposal suggested at https://github.com/bitcoin/bitcoin/pull/11775/files#diff-e8d9e22d9683f73a9fb8399be0dab640R145

TheBlueMatt · 2018-03-27T21:13:25Z

Rebased.

jimpo

Concept ACK. Overall, this looks awesome and simplifies a lot of code. I'm all for decoupling stuff through the ValidationInterface.

I did find it hard to review because it is so large and would prefer it be broken up into fee estimate stuff and non-fee estimate specific refactors.

Also, there's a typo in the commit title of "Make fee estimation not need CTxMemPoolEntry is block processing"

jimpo · 2017-11-29T03:31:46Z

src/txmempool.h

commit: Split removeRecursive into calculate/remove steps

style: Capitalize function name.

This PR was created long before the introduction of that part of the style guide, so it matches the capitalization of removeRecursive.

jimpo · 2018-04-02T17:23:54Z

src/txmempool.cpp

commit: Split removeRecursive into calculate/remove steps

Should this AssertLockHeld?

Done (bonus: also fixed the whitespace issues in that commit).

jimpo · 2018-04-02T17:30:57Z

src/txmempool.cpp

commit: Track the set of transactions removed/conflicted in removeForBlock

I think this reserve call could be less performant than skipping it if there are a sufficient number of conflicting inputs. https://stackoverflow.com/a/1742961

Good catch. Removed.

jimpo · 2018-04-02T18:08:13Z

src/wallet/wallet.cpp

commit: BlockConnected(vtxConflicted) -> New MempoolInterface callback

Why not just move that logic here from BlockConnected as well?

Because we still need it in BlockConnected - there may be txn that weren't in our mempool that got confirmed, so copying it would be useless.

jimpo · 2018-04-02T18:14:45Z

src/init.cpp

commit: Add txn_replaced to Added callback, remove lReplaced ATMP arg

nit: Space after if

jimpo · 2018-04-02T19:06:06Z

src/policy/fees.cpp

commit: Make fee estimation not need CTxMemPoolEntry is block processing

I'd move this erase immediately after the removeTx above and define double fee_per_k = (double)pos->second.m_fee_per_k; as a variable up there as well.

Would also need to copy blockHeight as well, at which point you'd just be copying the whole struct so that you can remove it.

Yeah, I think copying the struct for the sake of simplifying the control flow (by only erasing in one place) is actually good. Also would just make it more readable to have a descriptive variable name like tx_info instead of pos->second.

jimpo · 2018-04-02T19:11:06Z

src/policy/fees.h

commit: Make fee estimation not need CTxMemPoolEntry is block processing

I don't really like that this method has the same name as the other removeTx because it's responsibilities are different.

Happy to rename it, but honestly I dont remember what the different responsibilities are. Its been so long since I wrote this I dont know how it works anymore. Any suggestions?

Actually, it's fine leaving this one as is because in the later commit (Process fee estimates in the background via MempoolInterface), the other removeTx gets renamed to removeTxNotInBlock.

Not really necessary, but maybe rename the other function to removeTxNotInBlock in this commit instead of a later commit to avoid the confusion here.

jimpo · 2018-04-02T20:10:45Z

src/policy/fees.cpp

commit: Track witness hash malleation in fee estimator

Why don't we want to record the blocks to confirm in this case?

Because we don't know how long its been around/it waited.

I don't understand. We have the record of when the tx entered the mempool (albeit with different witness data). How does the malleated witness change things?

Because clearly there was a propagation issue. We dont know whether the lower-feerate version (or some other version) was the one most miners saw or the higher-feerate version. Better to just ignore it.

jimpo · 2018-04-02T20:22:18Z

src/test/policyestimator_tests.cpp

commit: Process fee estimates in the background via MempoolInterface

Use SyncWithValidationInterfaceQueue?

This was written before that existed. Fixed.

jimpo · 2018-04-02T20:23:09Z

src/policy/fees.h

commit: Process fee estimates in the background via MempoolInterface

Make the callbacks protected?

sdaftuar · 2018-04-10T19:42:46Z

I've started reviewing and testing the first few commits in this PR, but I already agree with @jimpo above, that this would be easier to review if it was split up.

Perhaps we could first have a PR that changed the callback interface around, and then another PR that reworks the fee estimator to take advantage of the new interface?

(also this needs rebase)

ajtowns · 2018-04-12T02:50:02Z

ConceptACK for what it's worth, but definitely split this up -- it overflows my mental stack, and both times I've tried to review it it's already had conflicts with master that make a review not crazy useful...

Breaking out the trivial bits, at least "Clarify validationinterface notification ordering" and "Remove useless scoped (sic?) in AcceptToMemoryPool" seems like a good first move. Maybe "Split removeRecursive" as well. Otherwise just splitting the remaining patches into mempool/validationinterface vs fee-estimation focused seems plausible.

jnewbery · 2018-04-12T14:20:21Z

I haven't looked at the changes but a +700/-600 diff is somewhat intimidating.

I'd love to help you make progress on this and will happily review PRs that are a subset of this.

promag · 2018-04-12T14:38:21Z

Tested ACK.

I'm kidding please please split up 👍

maflcko · 2018-04-12T15:16:23Z

@BugTheBlueMatt ^

TheBlueMatt · 2018-04-13T18:59:22Z

Now based on #12979.

Because many listeners (eg wallet) will want both types of events to be well-ordered, they are parallel and connected on the backend. However, they are exposed separately to clients to separate the logic (and because, hopefully, eventually, they can be exposed to external clients of Bitcoin Core via libconsensus or similar).

This removes the whole ConnectTrace object, which may make it slightly harder to remove the unbounded-memory-during-reorg bug by throwing blocks out of memory and re-loading them from disk later. Comments are added to validationinterface to note where this should likely happen instead of ConnectTrace.

This makes the processBlock function in CBlockPolicyEstimator much more simlar to MempoolInterface's MempoolUpdatedForBlockConnect callback.

There are lots of better things we can do, but this is much better than it was.

TheBlueMatt force-pushed the 2017-09-background-feeest branch 2 times, most recently from 1e08182 to 88de71e Compare November 27, 2017 20:24

fanquake added TX fees and policy Validation labels Nov 27, 2017

TheBlueMatt force-pushed the 2017-09-background-feeest branch from 88de71e to 50abdd3 Compare December 13, 2017 22:33

TheBlueMatt force-pushed the 2017-09-background-feeest branch 2 times, most recently from 3cd22de to 455585f Compare December 15, 2017 23:16

TheBlueMatt force-pushed the 2017-09-background-feeest branch from 455585f to 14b299f Compare March 27, 2018 21:13

TheBlueMatt force-pushed the 2017-09-background-feeest branch from 14b299f to 68d7445 Compare March 28, 2018 18:09

jimpo reviewed Apr 2, 2018

View reviewed changes

TheBlueMatt force-pushed the 2017-09-background-feeest branch from 68d7445 to 4c526df Compare April 5, 2018 18:02

maflcko mentioned this pull request Apr 11, 2018

Add ProcessOrphans (move-only) #12935

Closed

TheBlueMatt mentioned this pull request Apr 13, 2018

Split validationinterface into parallel validation/mempool interfaces #12979

Closed

TheBlueMatt force-pushed the 2017-09-background-feeest branch from 4c526df to 4e82ca6 Compare April 13, 2018 18:59

TheBlueMatt force-pushed the 2017-09-background-feeest branch 2 times, most recently from bbfb971 to eaa71b2 Compare April 17, 2018 15:26

ryanofsky mentioned this pull request Apr 30, 2018

[RFC] I Have a Hammer! (Replace parts of ui_interface with validationinterface) #11856

Closed

TheBlueMatt added 2 commits May 2, 2018 14:09

Clarify validationinterface notification ordering

bad68e9

TheBlueMatt added 3 commits May 2, 2018 14:09

Pass MemPoolRemovalReason out through TransactionRemovedFromMempool

07095f3

Split removeRecursive into calculate/remove steps

ec0f81f

Track the set of transactions removed/conflicted in removeForBlock

1ea56d1

TheBlueMatt force-pushed the 2017-09-background-feeest branch from eaa71b2 to 8bd9d71 Compare May 2, 2018 18:09

TheBlueMatt added 9 commits May 2, 2018 14:12

Remove boost::signals from txmempool, call GetMainSignals() directly

4da0342

Remove useless scope in AcceptToMemoryPoolWorker

1520506

Add txn_replaced to Added callback, remove lReplaced ATMP arg

53b90df

Pass new block height through MempoolUpdatedForBlock

49f8f57

Make fee estimation not need CTxMemPoolEntry is block processing

6864a15

This makes the processBlock function in CBlockPolicyEstimator much more simlar to MempoolInterface's MempoolUpdatedForBlockConnect callback.

Process fee estimates in the background via MempoolInterface

a734d1d

Handle reorgs much better in fee estimator.

660190c

There are lots of better things we can do, but this is much better than it was.

Track witness hash malleation in fee estimator

6697c02

TheBlueMatt force-pushed the 2017-09-background-feeest branch from 8bd9d71 to 6697c02 Compare May 2, 2018 18:15

maflcko added the Needs rebase label Jun 6, 2018

TheBlueMatt closed this Jun 14, 2018

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

glozow added the Up for grabs label Nov 18, 2022

fanquake removed the Up for grabs label Aug 30, 2023

Move fee estimator into validationinterface/cscheduler thread #11775

Move fee estimator into validationinterface/cscheduler thread #11775

Uh oh!

Conversation

TheBlueMatt commented Nov 27, 2017

Uh oh!

TheBlueMatt commented Nov 27, 2017

Uh oh!

TheBlueMatt commented Dec 13, 2017

Uh oh!

theuni commented Dec 19, 2017

Uh oh!

TheBlueMatt commented Dec 19, 2017

Uh oh!

TheBlueMatt commented Mar 27, 2018

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Apr 10, 2018

Uh oh!

ajtowns commented Apr 12, 2018

Uh oh!

jnewbery commented Apr 12, 2018

Uh oh!

promag commented Apr 12, 2018

Uh oh!

maflcko commented Apr 12, 2018

Uh oh!

TheBlueMatt commented Apr 13, 2018

jimpo Apr 2, 2018 •

edited

Loading