22335474d768f99067856173ff2764b6db753f67 net: format peer+addr logs with `LogPeer` (Lőrinc)
e55ea534f74b2dd2a81e8f9972554303f7cf9c20 test: add pre-`LogPeer` net log assertion (Lőrinc)
736b17c0f0f23dca78e7266be6c98b24b0eda210 log: fix minor formatting in debug logs (Lőrinc)
9cf82bed3205d8405f56838d0e2ca0c72023a081 log: show placeholders for missing peer fields (Lőrinc)
Pull request description:
This is an alternative to #34293, but aims to address the remaining logging inconsistencies more broadly.
It extends the example fixed there to every instance, restores the original separator behavior, applies it consistently via a single helper, and adds tests for `logips` (covering both current and new behavior).
### Problem
After #28521 centralized peer address logging into `CNode::LogIP()`, the original comma separator before `peeraddr=` was lost, resulting in inconsistent formatting across net (and recent private broadcast) logs.
Some lines also had double spaces, empty fields, or mismatched format specifiers.
### Fix
Introduces `CNode::LogPeer(bool)` which always emits `peer=<id>` and, when `-logips=1`, appends `, peeraddr=<addr>`. This eliminates hand-rolled separators and makes peer identification predictable.
Minor issues (double spaces, empty placeholders, format specifiers) are fixed along the way in separate commits.
### Reproducer
Run with `-debug=net -logips=1` and observe peer log lines now show `peer=<id>, peeraddr=<addr>` (comma-separated). The new assertion in `feature_logging.py` automates this check.
ACKs for top commit:
naiyoma:
ACK 22335474d768f99067856173ff2764b6db753f67
vasild:
ACK 22335474d768f99067856173ff2764b6db753f67
sedited:
ACK 22335474d768f99067856173ff2764b6db753f67
Tree-SHA512: 562262a58c3042f139099ff4c41e3fc6a97505fe9603c2bf700a97fd0aa052954b47c14da0e50c1fc311db1ae6c04e6a92156c9b85e25c777a637b7766c1dafe
We expect this to be rare in practice, and to not be the
usual state of the mempool. If we we detect non-optimal
ordering after a DoWork() invocation, allow this to be
observed in MEMPOOL logs.
Tidy a few debug log strings to avoid double spaces, concatenated status words, and mismatched format specifiers.
Co-authored-by: naiyoma <lankas.aurelia@gmail.com>
This adds an std::function<strong_ordering(Ref&,Ref&)> argument to the
MakeTxGraph function, which can be used by the caller (e.g., mempool
code) to provide a fallback order to TxGraph.
This is just preparation; TxGraph does not yet use this fallback order
for anything.
Instead of returning a TxGraph::Ref from TxGraph::AddTransaction(),
pass in a TxGraph::Ref& which is updated to refer to the new transaction
in that graph.
This cleans up the usage somewhat, avoiding the need for dummy Refs in
CTxMemPoolEntry constructor calls, but the motivation is that a future
commit will allow a callback to passed to MakeTxGraph to define a
fallback order on the transaction objects. This does not work when a
Ref is created separately from the CTxMemPoolEntry it ends up living in,
as passing the newly-created Ref to the callback would be UB before it's
emplaced in its final CTxMemPoolEntry.
Preparation for a future commit where kernel's dependency
on logging.cpp is removed completely.
Replace usage of logging\.h with util/log\.h where it
suffices, and fix wrong includes according to iwyu.
This is likely slightly slower, but this was the last place we were using
epochs instead of sets to deduplicate, and this is only used by the RPC code
and in tests, and should not be CPU-performance critical. Eliminating this
allows us to save 8 bytes in CTxMemPoolEntry.
Co-Authored-By: Pieter Wuille <bitcoin-dev@wuille.net>
Also improve test coverage for removeForReorg by creating a scenario where
there are in-mempool descendants that are only invalidated due to an in-mempool
parent no longer spending a mature coin.
Previously we would sanity check the -maxmempool configuration based on a
multiple of the descendant size limit, but with cluster mempool the maximum
evicted size is now the cluster size limit, so use that instead.
Also allow -maxmempool=0 in general (and not just if
-limitdescendantsize/-limitclustersize is set to 0).
We use CompareMiningScoreWithTopology() for sorting transaction announcements
during tx relay, and we use GetSortedScoreWithTopology() in
CTxMemPool::check().
Calculating mempool ancestors for a new transaction should not be done until
after cluster size limits have been enforced, to limit CPU DoS potential.
Achieve this by reworking TRUC and RBF validation logic:
- TRUC policy enforcement is now done using only mempool parents of
new transactions, not all mempool ancestors (note that it's fine to calculate
ancestors of in-mempool transactions, if the number of such calls is
reasonably bounded).
- RBF replacement checks are performed earlier (which allows for checking
cluster size limits earlier, because cluster size checks cannot happen until
after all conflicts are staged for removal).
- Verifying that a new transaction doesn't conflict with an ancestor now
happens later, in AcceptSingleTransaction() rather than in PreChecks(). This
means that the test is not performed at all in AcceptMultipleTransactions(),
but in package acceptance we already disallow RBF in situations where a
package transaction has in-mempool parents.
Also to ensure that all RBF validation logic is applied in both the single
transaction and multiple transaction cases, remove the optimization that skips
the PackageMempoolChecks() in the case of a single transaction being validated
in AcceptMultipleTransactions().
Now that ancestor calculation never fails (due to ancestor/descendant limits
being eliminated), we can eliminate the error handling from
CalculateMemPoolAncestors.
With the descendant size limits removed, replace the concept of "max number of
descendants of any ancestor of a given tx" with the cluster count of the cluster
that the transaction belongs to.
The mempool clusters and linearization permit sorting the mempool topologically
without making use of ancestor counts (as long as the graph is not oversized).
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Previously, transaction batches were first sorted by ancestor count and then
feerate, to ensure transactions are announced in a topologically valid order,
while prioritizing higher feerate transactions. Ancestor count is a crude
topological sort criteria, so replace this with linearization order so that the
highest feerate transactions (as would be observed by the mining algorithm) are
relayed before lower feerate ones, in a topologically valid way.
This also fixes a test that only worked due to the ancestor-count-based sort
order.
With a total ordering on mempool transactions, we are now able to calculate a
transaction's mining score at all times. Use this to improve the RBF logic:
- we no longer enforce a "no new unconfirmed parents" rule
- we now require that the mempool's feerate diagram must improve in order
to accept a replacement
- the topology restrictions for conflicts in the package rbf setting have been
eliminated
Revert the temporary change to mempool_ephemeral_dust.py that were previously
made due to RBF validation checks being reordered.
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>, glozow <gloriajzhao@gmail.com>
Rather than evicting the transactions with the lowest descendant feerate,
instead evict transactions that have the lowest chunk feerate.
Once mining is implemented based on choosing transactions with highest chunk
feerate (see next commit), mining and eviction will be opposites, so that we
will evict the transactions that would be mined last.
