log: Properly log txs rejected from mempool #18990

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2005-logMempoolRej changing 2 files +8 −5

MarcoFalke commented at 2:49 PM on May 16, 2020: member
Currently CheckTxInputs rejections from the mempool are the only rejections that log directly and unconditionally to debug.log instead of leaving it to the caller. This has multiple issues:
- A rejected RPC transaction will log a redundant failure reason to debug log. All other failures are merely reported to the RPC user.
- A rejected p2p transaction will log the failure twice. Once with the MEMPOOLREJ flag, and once unconditionally.
- A rejected orphan transaction will log no failure.
Fix all issues by simply returning the state to the caller, like it is done for all other rejections.

The patch includes whitespace fixups to highlight relevant parts of the codebase and simplify review.
log: Properly log txs rejected from mempool fa9f20b647
DrahtBot added the label P2P on May 16, 2020
DrahtBot added the label Utils/log/libs on May 16, 2020
DrahtBot added the label Validation on May 16, 2020
naumenkogs commented at 7:11 PM on May 16, 2020: member

utACK fa9f20b6477a206adf5089398803b45d1a114b6f

in src/net_processing.cpp:1959 in fa9f20b647

1953 | @@ -1954,7 +1954,10 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::set<uin
1954 |                  if (MaybePunishNodeForTx(fromPeer, orphan_state)) {
1955 |                      setMisbehaving.insert(fromPeer);
1956 |                  }
1957 | -                LogPrint(BCLog::MEMPOOL, "   invalid orphan tx %s\n", orphanHash.ToString());
1958 | +                LogPrint(BCLog::MEMPOOL, "   invalid orphan tx %s from peer=%d. %s\n",
1959 | +                    orphanHash.ToString(),
1960 | +                    fromPeer,

luke-jr commented at 7:23 PM on June 2, 2020:

Is this something we want to log the peer for?

jnewbery commented at 12:57 PM on July 14, 2020:

yes (and we already do, in the MaybePunishNodeForTx() call)

rajarshimaitra commented at 6:05 PM on July 3, 2020: contributor

Concept ACK. Compiled and ran tests. fa9f20b
DrahtBot commented at 7:03 PM on July 12, 2020: 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:
- #19498 (Tidy up ProcessOrphanTx by jnewbery)
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.
jnewbery commented at 12:57 PM on July 14, 2020: member

code review ACK fa9f20b6477a206adf5089398803b45d1a114b6f
MarcoFalke commented at 1:22 PM on July 14, 2020: member

re-run ci
MarcoFalke closed this on Jul 14, 2020
MarcoFalke reopened this on Jul 14, 2020
MarcoFalke merged this on Jul 14, 2020
MarcoFalke closed this on Jul 14, 2020
MarcoFalke deleted the branch on Jul 14, 2020
sidhujag referenced this in commit 789bfc0683 on Jul 14, 2020
deadalnix referenced this in commit 6866e39368 on Aug 31, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

P2P Validation Utils/log/libs

log: Properly log txs rejected from mempool #18990

Conflicts