mempool: Skip estimator if block is older than X #16066

promag commented at 2:32 PM on May 21, 2019: member

While profiling IBD it seems to be a waste of resources to keep the estimator updated from the start. Currently, with this change, the estimator is not updated if the block is older than 2 days (MAX_FEE_ESTIMATOR_BLOCK_AGE). This is temporary, a proper age should be used so that the estimator result is identical.

Please advice, opening as a draft.

Stack trace before and after: Screenshot 2019-05-21 at 16 12 02

Screenshot 2019-05-21 at 16 13 32

mempool: Update estimator if verification progress > 90% 8854ebbec6

practicalswift commented at 2:57 PM on May 21, 2019: contributor

@promag Do you have any IBD measurements before vs after this change to share?

promag commented at 3:16 PM on May 21, 2019: member

@practicalswift updated OP.

in src/validation.cpp:2462 in 8854ebbec6 outdated

2458 | @@ -2459,7 +2459,7 @@ bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainp
2459 |      int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4;
2460 |      LogPrint(BCLog::BENCH, "  - Writing chainstate: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime5 - nTime4) * MILLI, nTimeChainState * MICRO, nTimeChainState * MILLI / nBlocksTotal);
2461 |      // Remove conflicting transactions from the mempool.;
2462 | -    mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight);
2463 | +    mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, GuessVerificationProgress(chainparams.TxData(), pindexNew) > 0.9);

MarcoFalke commented at 4:02 PM on May 21, 2019:

We shouldn't use a function that is only used to print debug statements in validation. Couldn't you replace this with a check if (this_block_height < .9*total_header_height)?

MarcoFalke commented at 4:03 PM on May 21, 2019: member

Concept ACK if the change is correct and speeds up ibd

DrahtBot added the label Mempool on May 21, 2019

DrahtBot added the label Validation on May 21, 2019

promag renamed this:
~~mempool: Update estimator if verification progress > 90%~~
mempool: Update estimator if block age is up to 2 days
on May 21, 2019

promag commented at 5:00 PM on May 21, 2019: member

@MarcoFalke updated to take into account block age instead.

DrahtBot commented at 5:00 PM on May 21, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14193 (validation: Add missing mempool locks by MarcoFalke)
#13949 (Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency by Empact)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/validation.h:111 in d9d41f210e outdated

 106 | @@ -107,6 +107,8 @@ static const int64_t BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 500000;
 107 |  static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60;
 108 |  /** Maximum age of our tip in seconds for us to be considered current for fee estimation */
 109 |  static const int64_t MAX_FEE_ESTIMATION_TIP_AGE = 3 * 60 * 60;
 110 | +/** Maximum block age in seconds for us to be considered suitable for fee estimator */
 111 | +static const int64_t MAX_FEE_ESTIMATOR_BLOCK_AGE = 48 * 60 * 60;

MarcoFalke commented at 6:19 PM on May 21, 2019:

Should explain where this magic number comes from and why it is a safe choice considering the other constants in policy/fees.h. Ideally you'd express it using existing constants

promag commented at 7:29 PM on May 21, 2019:

No magic here, just picked 2 days .. I think it should be a value that would produce similar fee rates right?

thijstriemstra commented at 12:05 AM on May 22, 2019:

2 days is still magic ;) "for us to be considered suitable" also sounds confusing to me, could you rephrase this?

promag commented at 12:15 AM on May 22, 2019:

I'll change to a reasonable value, meanwhile I'm waiting for concept ACK.

promag renamed this:
~~mempool: Update estimator if block age is up to 2 days~~
mempool: Skip estimator if block is older than X
on May 21, 2019

fixup: Only update fee estimator if block age is up to 2 days e6343d866d

promag force-pushed on May 21, 2019

morcos commented at 7:23 PM on May 22, 2019: member

I give this idea a concept ACK, but I think it requires more complicated changes to work correctly. After a cursory review, I think basic functionality would have to be something like this:

We should not completely skip CBlockPolicyEstimator::processBlock but instead pass into that function some notion of whether the individual block transactions need to be processed. Call this track_block_transactions.

If track_block_transactions is false, then processBlock can just do the quick initial work of decaying existing estimates and skip the time consuming processing of the transactions within the block. Presumably this will give us almost all of the speedup hoped for, if not, maybe we can look for further optimizations.

There is already an existing function used for determining whether incoming transactions should be tracked, IsCurrentForFeeEstimation. I think that the notion we want for track_block_transactions is that it starts false (assuming IsCurrentForFeeEstimation starts false) and then latches to true the first time IsCurrentForFeeEstimation goes true. IsCurrentForFeeEstimation can go back to false again, but its important that once mempool transactions have started being tracked that we never go back to false for track_block_transactions.

I seem to remember someone else working on this exact improvement in the past, maybe @theuni?

(EDIT: It may be even easier than suggested. The CBlockPolicyEstimator can probably just internally use !mapMemPoolTxs.empty() as a proxy for track_block_transactions)

promag commented at 7:30 PM on May 22, 2019: member

@morcos thanks! Actually IIRC decaying does cost and I think it's a bottleneck in reindex. Later I'll update profile results to see what I mean.

MarcoFalke commented at 7:39 PM on May 22, 2019: member