tx_extra limitation fix [release-v0.18] #8806
Conversation
|
|
||
| if(tvc[i].m_verifivation_failed) | ||
| {MERROR_VER("Transaction verification failed: " << results[i].hash);} | ||
| else if(tvc[i].m_verifivation_impossible) | ||
| {MERROR_VER("Transaction verification impossible: " << results[i].hash);} | ||
|
|
||
| if(tvc[i].m_added_to_pool) | ||
| if(tvc[i].m_added_to_pool && (results[i].tx.extra.size() <= MAX_TX_EXTRA_SIZE)) |
There was a problem hiding this comment.
Why is this check necessary? m_added_to_pool is only ever set after the tx.extra checks in add_new_tx. In other words, it seems redundant because m_added_to_pool will be false if tx.extra is too big anyways.
There was a problem hiding this comment.
It can happen if kept_by_block is true, and then I get tx spam in p2pool logs - all big tx_extra transactions get added to a new block and leak into ZMQ notifications, but they shouldn't because ZMQ notifications are supposed to notify about mempool only.
There was a problem hiding this comment.
@jeffro256 I just did a quick test by adding some logging: https://paste.debian.net/hidden/15de343a/
I can see both places filtering transactions from being sent to ZMQ subscribers, so it's correct. tvc[i].m_added_to_pool can be true but we still end up in the else branch, and the debug logging shows it.
There was a problem hiding this comment.
Correction: https://paste.debian.net/hidden/d345d788/ - now I see only the first place filtering transactions, I'll keep running it to see if the second one triggers or not.
There was a problem hiding this comment.
if(tvc[i].m_added_to_pool...
but they shouldn't because ZMQ notifications are supposed to notify about mempool only.
These two things seem to be contradicting each other ^. Is there a deeper problem here with the meaning of m_added_to_pool?
There was a problem hiding this comment.
When a new block arrives, monerod downloads missing transactions from this block, adds them to mempool for a split second and then removes them from mempool and adds them to the blockchain. All this juggling triggers ZMQ notifications which are useless to subscribers. So m_added_to_pool is technically correct here, but useless in practice.
There was a problem hiding this comment.
Since downloaded transactions are "forced into" the mempool by kept_by_block, can we not generalize this condition to tvc[i].m_added_to_pool && tx_relay != relay_method::block? This would exclude any internal blockchain shufflings inside the pool, not just for tx_extra.
There was a problem hiding this comment.
I'll run with debug logging to confirm that it's only relay_method::block there, and if it is, I'll change to && tx_relay != relay_method::block
There was a problem hiding this comment.
@jeffro256 I just realized that kept_by_block can make transactions stay in mempool for real, and be actually valid to be mined again, for example after a reorg. So it's a bad idea to change to tx_relay != relay_method::block, it's better to filter only big tx_extra.
|
Follow up PR: it looks like this same problem can occur for txs with too-low fees (a bit surprising it has never been encountered - a testament to the fee algorithm's adoption perhaps). Minimum fee is a relay-only rule like the tx extra cap. |
SChernykh
force-pushed
the
tx_extra_patch
branch
from
758b138
to
f3298cf
Compare
| drop_connection(context, false, false); | ||
| // Don't drop connections which send transactions with too big tx_extra | ||
| // It will only cause updated nodes to isolate themselves from the rest of the network | ||
| // This check can be removed as soon as updated nodes are the majority (both by hashrate and by total count) |
There was a problem hiding this comment.
// This check can be removed as soon as updated nodes are the majority (both by hashrate and by total count)
It's not clear to me this is the best strategy/philosophy for managing the network (i.e. blacklisting nodes that don't align with your node's relay rules), you may want to bring it up in -dev.
There was a problem hiding this comment.
"can be removed" != "must be removed", not even "should be removed"
There was a problem hiding this comment.
"can be removed" = it's fine to remove it, but I am saying that's not necessarily true.
|
|
||
| if(tvc[i].m_verifivation_failed) | ||
| {MERROR_VER("Transaction verification failed: " << results[i].hash);} | ||
| else if(tvc[i].m_verifivation_impossible) | ||
| {MERROR_VER("Transaction verification impossible: " << results[i].hash);} | ||
|
|
||
| if(tvc[i].m_added_to_pool) | ||
| if(tvc[i].m_added_to_pool && (results[i].tx.extra.size() <= MAX_TX_EXTRA_SIZE)) |
There was a problem hiding this comment.
if(tvc[i].m_added_to_pool...
but they shouldn't because ZMQ notifications are supposed to notify about mempool only.
These two things seem to be contradicting each other ^. Is there a deeper problem here with the meaning of m_added_to_pool?
- Fixed connection drop when a big tx_extra is received from another node - Double check for MAX_TX_EXTRA_SIZE before sending out ZMQ txpool_event notifications to properly mark them as `invalid`
SChernykh
force-pushed
the
tx_extra_patch
branch
from
f3298cf
to
df40e89
Compare
SChernykh
changed the title
|
Replaced with #8813 |
invalidThe second fix (ZMQ notifications) is to stop the notification spam when a new block with big tx_extra transactions is received - it brings unneccessary load to p2pool miners.