-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Reduce MAX_INV_SZ so wtx invs fit within a network message #6643
base: master
Are you sure you want to change the base?
Conversation
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.
utACK
I don't understand the rationale. Why does this need to be changed, more precisely? |
With the larger wtxid In Zebra it was possible (but unlikely) if we got around 30,000 new v5 transactions in the mempool. |
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.
NACK. A node that sends an inv with more than MAX_INV_SZ
entries will be treated as misbehaving:
Lines 6899 to 6904 in 7dc1f63
if (vInv.size() > MAX_INV_SZ) | |
{ | |
LOCK(cs_main); | |
Misbehaving(pfrom->GetId(), 20); | |
return error("message inv size() = %u", vInv.size()); | |
} |
Therefore reducing
MAX_INV_SZ
is an incompatible p2p protocol change that could cause nodes unaware of the change to get banned unjustly.
(The "Changes requested" is to block merging for now, until we've discussed this.)
If it can, this isn't the correct fix. |
We had a close look at this. It appears as though it might technically be possible for zcashd to send a protocol message larger than the limit if there is a lot of mempool churn without |
I think an over-sized network message can happen if:
It might also be possible for transaction gossips to reach this size, if the user increases the mempool limits. But I don't know how this is implemented in I'm happy to close this PR, and leave the proper fix to you. I don't think I understand For now, Zebra is going to impose a lower limit so we don't send over-sized messages, and that also means we'll temporarily disconnect from nodes that send over this limit. Happy to negotiate the exact limit and how it's implemented, but Zebra needs to get an initial fix in soon. |
Actually, it turns out we need to split the inbound and outbound limits, so that's done. I think it's still possible we might disconnect from nodes that send a lot of |
I propose setting the outbound limit to 30000. Then we have |
Just to make sure we're on the same page, the current effective limits for regular network requests are:
An The problem with reducing |
MAX_INV_SZ is too large for a network message filled with wide transaction IDs:
https://zips.z.cash/zip-0239#deployment
But this is unlikely to happen with the default mempool config, because ZIP-401 limits the number of transactions in the mempool to 8,000.