Mempool: persist mempoolminfee accross restarts

ismaelsadeeq commented at 9:33 am on June 12, 2023: member

Follow-up #27622 See –> Comment Currently, there is no persistence of mempoolminfee As mentioned in the comment

persist mempoolminfee across restarts to at least tell user a plausible value to stay in the mempool for a bit

The suggestions outlined in the comment are implemented in this pull request. The mempoolminfee will now be persisted on shutdown and periodically in the event of an unclean shutdown. Upon restart, the previous mempoolminfee value will be remembered, and any transactions with fees below the persisted mempoolminfee will be rejected from entering the mempool.

If a user passes the -minrelaytxfee, it will be utilized as the mempoolminfee, just as it was prior to this pull request. When reading the persisted mempoolminfee, we take the max of the persisted value and the default minimum fee of 1000 satoshis, ensuring that the remembered mempoolminfee value will not be less than 1000 satoshis.

DrahtBot commented at 9:33 am on June 12, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28368 (Fee Estimator updates from Validation Interface/CScheduler thread by ismaelsadeeq)

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.

DrahtBot added the label Mempool on Jun 12, 2023

in src/node/mempool_persist_args.cpp:27 in e57d0781a0 outdated

20@@ -21,3 +21,8 @@ fs::path MempoolPath(const ArgsManager& argsman)
21 }
22 
23 } // namespace node
24+
25+fs::path MemPoolMinFeePath(const ArgsManager& argsman)
26+{
27+    return argsman.GetDataDirNet() / "mempool_min_fee.dat";

ajtowns commented at 9:44 am on June 12, 2023:

Why add this as a separate file, rather than adding it to the end of the fee estimates data file which is already updated periodically and ignored if out of date?

Adding a method to the mempool to set the min fee based on that seems plausible? (Perhaps make it conditional on the new value being higher than the current minfee, and the mempool being less than 95% full?)

ismaelsadeeq commented at 9:20 am on June 13, 2023:

Thank you. Yes, I agree that adding it at the end of fee_estimates.dat would indeed reduce redundancy. I would update that.

Regarding making it conditional to read the persisted mempoolminfee, could you please clarify the scenarios in which we might want to use the default mempoolminfee value? In this, it is already conditional to remember the mempoolminfee as stale mempoolminfee will not be read at all.

ajtowns commented at 1:42 am on June 14, 2023:

I guess as long as it’s only set shortly after startup (while the mempool is still loading?), it’s probably fine?

ismaelsadeeq commented at 11:43 am on June 14, 2023:

I updated the commits, it’s now flushing to fee_estimates.dat

in doc/files.md:67 in 944350c69a outdated

63@@ -64,6 +64,7 @@ Subdirectory       | File(s)               | Description
64 `./`               | `guisettings.ini.bak` | Backup of former [GUI settings](#gui-settings) after `-resetguisettings` option is used
65 `./`               | `ip_asn.map`          | IP addresses to Autonomous System Numbers (ASNs) mapping used for bucketing of the peers; path can be specified with the `-asmap` option
66 `./`               | `mempool.dat`         | Dump of the mempool's transactions
67+`./`               | `mempool_min_fee.dat` | Stores the minimum fee rate of a transaction that will be accepted into the mempool

ajtowns commented at 9:50 am on June 12, 2023:

The idea is to update it periodically, and writing the full mempool to disk regularly may be unappealing.

maflcko commented at 10:11 am on June 12, 2023:

(This was in reply to my question why not to save it to mempool.dat, which was accidentally deleted)

writing the full mempool to disk regularly may be unappealing

Probably off-topic here, but if we assume that the fee estimates must be written regularly due to unclean shutdowns, then the mempool should be as well, no?

ismaelsadeeq commented at 9:44 am on June 13, 2023:

It appears that taking the approach of dumping to fee_estimates.dat and implementing a method to set mempoolminfee based on the persisted value is a better option.

Your perspective on persisting the mempool periodically. I understand the concern about remembering the fee estimates and mempoolminfee without the mempool itself. However, I would like to seek further insight into the potential consequences that could arise from this, as dumping the mempool periodically might be unappealing

willcl-ark commented at 1:00 pm on June 13, 2023:

I would agree that it makes sense to dump into fee_estimates.dat.

DrahtBot added the label CI failed on Jun 12, 2023

in src/txmempool.cpp:435 in 16088103c0 outdated

449+            throw std::runtime_error("MemPool minimum fee corrupt");
450+        }
451+        /** The current MemPool minimum fee will be determined by taking the maximum of
452+         *  the persisted MemPool minimum fee and the default minimum fee.
453+         **/
454+        rollingMinimumFeeRate = std::max(mempool_min_fee, GetMinFee().GetFeePerK());

willcl-ark commented at 12:54 pm on June 13, 2023:

An alternative could be to also set rollingMinimumFeeRate, and then also setlastRollingFeeUpdate to the timestamp of the min fee dump, and then just let `GetMinFee() run its exponential backoff algorithm as per normal?

ismaelsadeeq commented at 11:41 am on June 14, 2023:

Thank you. Changed the approach to dumping in fee_estimates.dat the timestamp is not available there.

in src/txmempool.cpp:413 in 16088103c0 outdated

408+        LogPrintf("Minimum relay transaction fee has been set will be used as MemPool minimum fee. \n");
409+        return;
410+    }
411+    AutoFile min_fee_file{fsbridge::fopen(m_mempool_min_fee_file_path, "rb")};
412+
413+    // If the file is not found return early

willcl-ark commented at 12:56 pm on June 13, 2023:

nit: I think the code is clear enough here that comments aren’t really needed?

ismaelsadeeq commented at 11:42 am on June 14, 2023:

no longer there in the recent push

in src/txmempool.cpp:439 in 16088103c0 outdated

434+        filein >> file_timestamp;
435+        auto now = std::chrono::system_clock::now();
436+        uint64_t current_timestamp = TicksSinceEpoch<std::chrono::seconds>(now);
437+        uint64_t difference_in_hours = (current_timestamp - file_timestamp) / 3600;
438+
439+        // Do not read MemPool minimum fee if it's too old

willcl-ark commented at 12:56 pm on June 13, 2023:

nit: same here re. comments (and a few more places).

willcl-ark commented at 1:26 pm on June 13, 2023: member

I like the approach here and think persisting mempool min fee is useful. I also agree that re-using fee_estimates.dat would make sense for it, as it likely doesn’t need its own file.

Left a few nits and suggestions inline.

ismaelsadeeq force-pushed on Jun 13, 2023

in src/policy/fees.cpp:925 in 0ff51ae294 outdated

923-    FlushFeeEstimates();
924+    FlushFeeEstimates(mempool_min_fee);
925 }
926 
927-void CBlockPolicyEstimator::FlushFeeEstimates()
928+void CBlockPolicyEstimator::FlushFeeEstimates(const int64_t& mempool_min_fee)

glozow commented at 11:52 am on June 14, 2023:

0ff51ae294b2fd8106d7b877ba43746f448b6876: pass primitive types by value

ismaelsadeeq commented at 8:42 pm on June 19, 2023:

Done

in src/txmempool.h:755 in 0ff51ae294 outdated

743@@ -744,6 +744,13 @@ class CTxMemPool
744         return m_sequence_number;
745     }
746 
747+    /** Flush fee estimates and mempool minimum fee to disk*/
748+    void FlushFeeEstimatesAndMempoolMinFee() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
749+
750+    void SetMemPoolMinFee() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
751+
752+    int64_t GetMempoolMinFee() const EXCLUSIVE_LOCKS_REQUIRED(!cs);

glozow commented at 11:53 am on June 14, 2023:

0ff51ae294b2fd8106d7b877ba43746f448b6876 Why is this an int64_t instead of a CFeeRate or a CAmount?

ismaelsadeeq commented at 9:02 pm on June 19, 2023:

CFeeRate’s nSatoshisPerK is CAmount same int64_t.

ismaelsadeeq commented at 4:51 pm on July 25, 2023:

@glozow fixed now using CAmount Thank you

in src/txmempool.h:740 in 1afef831d9 outdated

743@@ -744,6 +744,13 @@ class CTxMemPool
744         return m_sequence_number;
745     }
746 
747+    /** Flush fee estimates and mempool minimum fee to disk*/
748+    void FlushFeeEstimatesAndMempoolMinFee() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
749+
750+    void SetMemPoolMinFee() const EXCLUSIVE_LOCKS_REQUIRED(!cs);

glozow commented at 12:01 pm on June 14, 2023:

Question: why also set this as the policy for tx validation? If the motivation is only to make sure it is considered for fee estimation, then why not just store it in CBlockPolicyEstimator? Given how much can happen in 60 hours, I’m unsure if it’s a good idea to just set this feerate without knowing what the current traffic looks like.

ismaelsadeeq commented at 11:45 am on June 15, 2023:

I think it should be considered for tx policy validation

While mempool is importing, node receives transactions well below the old mempoolminfee, enters into mempool

See issue

I agree with your point a lot can happen in 60 hours. I also agree with @willcl-ark comment to make it stricter for reloading mempoolminfee only if it’s an immediate restart.

I am taking this to draft while working on that

ismaelsadeeq commented at 8:49 pm on June 19, 2023:

I reduced the maximum age of a mempoolminfee to one hour.

ismaelsadeeq marked this as a draft on Jun 15, 2023

ismaelsadeeq force-pushed on Jun 19, 2023

DrahtBot removed the label CI failed on Jun 19, 2023

ismaelsadeeq marked this as ready for review on Jun 19, 2023

bitcoin deleted a comment on Jun 19, 2023

ismaelsadeeq commented at 3:24 pm on June 21, 2023: member

The issue is after an immediate restart.

Fee estimates will be remembered but mempoolminfee will fall back to m_incremental_relay_feerate 1000 sats. This will make propagating low-fee transactions accepted into the mempool.
fee_estimator.estimateSmartFee will return a non-0 fee rate below 1000 sats (Potentially because of those recently accepted transactions).
This makes the estimatesmartfee 1000 sats because we are taking std::max({feeRate, min_mempool_feerate, min_relay_feerate});

I also think this happens when low-fee transactions are propagating after restarts.

After some time the fee estimates will be back to normal.

So if we persisted mempoolminfee and read it only after immediate restarts, We will not accept low-fee transactions that will mess with the fee estimates.

I tested on signet and noticed the estimate after an immediate restart is the same as before. There aren’t low-fee transactions when I restarted.

Except if there is a reason for the mempoolminfee to fall back to 1000 sats after an immediate restart, we should not accept these transactions below the mempoolminfee to our mempool just because we restart.

ismaelsadeeq force-pushed on Jul 25, 2023

DrahtBot added the label Needs rebase on Aug 22, 2023

ismaelsadeeq force-pushed on Aug 23, 2023

DrahtBot removed the label Needs rebase on Aug 23, 2023

DrahtBot added the label CI failed on Aug 23, 2023

maflcko commented at 7:44 am on August 24, 2023: member

CI fails with:

0�[0m�[0;31mfeature_mempool_min_fee_persist.py --descriptors       | ✖ Failed  | 4 s

ismaelsadeeq force-pushed on Aug 25, 2023

DrahtBot removed the label CI failed on Aug 25, 2023

ismaelsadeeq commented at 1:59 pm on August 25, 2023: member

@MarcoFalke Rebased and fix CI failure

DrahtBot added the label CI failed on Sep 16, 2023

maflcko commented at 3:39 pm on October 4, 2023: member

CI still fails

test: reset mempool minimum fee with `minrelaytxfee` option.

This commit update tests that have the assumption that mempool
minimum fee will be reset to the default value of 1000 Satoshi after a restart,
to restart with a minrelaytxfee of 1000 satoshis, which will effectively sets
the mempool minimum fee back to its default value.

The changes made in this pull request invalidate the previous assumption, necessitating
this adjustment.

9ccba09f96

Mempool: add a new property to `MemPoolOptions`

min_relay_feerate_set: indicates whether
`minrelaytxfee` was configured on startup or not.

f6d5034f0b

Mempool: persist mempool minimum fee accross restart

During shutdown, the mempool minimum fee value is now written to
the 'fee_estimates.dat' file.

On startup, if the user has set the -minrelaytxfee, it will
be used as the mempool minimum fee. Otherwise, if fee_estimates.dat
file is not too old, the maximum value between the persisted mempool min fee
fee and the default mempool minimum fee will be used as the 'rollingMinimumFeeRate'.

The mempool minimum fee will be flushed periodically in case of an
unclean shutdown.

89502ba846

test: tests persistence of Mempool minimum fee accross restart

Ensure that mempool minimum fee will be flushed to disk
periodically and on shutdown, also tests that stale mempool
minimum fee will not be read.

652ee6a7f7

ismaelsadeeq force-pushed on Oct 6, 2023

ismaelsadeeq commented at 5:40 am on October 6, 2023: member

CI still fails

Should be fixed after this rebase. I am rethinking the approach taken in this PR, given that there is another PR that decouples the fee estimator from the mempool in progress. Is it more desirable to persist to mempool.dat but that does not flush periodically and will be undesirable to do so. will the initial approach of having its own file be better.

Also conceptually what do you think about this PR Thanks. @ajtowns @willcl-ark @glozow

DrahtBot removed the label CI failed on Oct 6, 2023

ismaelsadeeq commented at 5:43 pm on October 16, 2023: member

Closing this due to lack of conceptual motivation.

ismaelsadeeq closed this on Oct 16, 2023

bitcoin locked this on Oct 15, 2024

ismaelsadeeq deleted the branch on Nov 29, 2024

Mempool: persist mempoolminfee accross restarts #27859

Code Coverage

Reviews

Conflicts