-
Notifications
You must be signed in to change notification settings - Fork 844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dedicated log marker for invalid txs removed from the txpool #6826
Dedicated log marker for invalid txs removed from the txpool #6826
Conversation
Signed-off-by: Fabio Di Fabio <[email protected]>
090894d
to
ee7f0f7
Compare
…xpool Signed-off-by: Justin Florentine <[email protected]>
LOG.atInfo() | ||
.addMarker(INVALID_TX_REMOVED) | ||
.addKeyValue("txhash", pendingTransaction::getHash) | ||
.addKeyValue("txlog", pendingTransaction::toTraceLog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size of the tracelog and the raw data gives me concerns about this as a griefing vector. What if we separated those two fields into a debug call and leave hash and reason in at info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your concern is about the size of the logged message, then the way I set up the log with the marker and key-value, is indeed to avoid that this line is logged by default, and you need to explicitly configure log4j to enabled this log, and you can choose which of the keys to log.
LOG.atInfo() | ||
.addMarker(INVALID_TX_REMOVED) | ||
.addKeyValue("txhash", pendingTransaction::getHash) | ||
.addKeyValue("txlog", pendingTransaction::toTraceLog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same separation advice as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love Log4j use here, we should probably add a section on how to use this to https://besu.hyperledger.org/public-networks/how-to/monitor/logging
…dger#6826) Signed-off-by: Fabio Di Fabio <[email protected]> Signed-off-by: Justin Florentine <[email protected]> Co-authored-by: Justin Florentine <[email protected]> Signed-off-by: amsmota <[email protected]>
…dger#6826) Signed-off-by: Fabio Di Fabio <[email protected]> Signed-off-by: Justin Florentine <[email protected]> Co-authored-by: Justin Florentine <[email protected]> Signed-off-by: amsmota <[email protected]>
…dger#6826) Signed-off-by: Fabio Di Fabio <[email protected]> Signed-off-by: Justin Florentine <[email protected]> Co-authored-by: Justin Florentine <[email protected]>
PR description
Transactions that during block creation are found to be invalid are removed from the pool, and could be difficult to retrieve the content of the tx, so if needed for debug, with this PR is possible to enable the log the RLP of the invalid tx, along with some other info, these log lines have the marker
INVALID_TX_REMOVED
and the following fields, that can be used to format the log line as required:txhash
hash of the txtxlog
human readable log of the txreason
why the tx was invalidtxrlp
the RLP encoding of the txLog4j2 example configuration to enable the invalid tx log:
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests