limit local snark work broadcast #9011
Conversation
deepthiskumar
added
the
ci-build-me
src/lib/mina_lib/mina_lib.ml
Outdated
| ; peer_id= "" } | ||
| in | ||
| let rl = Network_pool.Transaction_pool.create_rate_limiter () in | ||
| log_rate_limiter_occasionally rl ~label:"local_transactions" ; |
There was a problem hiding this comment.
Nit: this also applies to rebroadcast of non-local transactions; perhaps the message should be transactions?
…ina into fix/snark-pool-rate-limit
There was a problem hiding this comment.
I'd misunderstood you when we were talking OOB about the inclusion_table. I think we still need it -- or something like it -- to prevent us from rebroadcasting snark work until its statement falls off the edge of the frontier.
I originally thought you were talking about keeping work around during a re-org, but I guess that's a far more complicated problem. Probably we should just leave the inclusion_table as is.
We rebroadcast work from the latest 10 blocks in the best chain. So this should be ok? mina/src/lib/network_pool/snark_pool.ml Line 535 in bfa47f1 |
There was a problem hiding this comment.
Code changes LGTM.
RE: the conversation about removing the inclusion_table -- seems to me that with the old behavior, we would not broadcast snark work that was included in a fork but not in the best tip chain. This sounds like it could, in theory, deadlock the scan state on a network if 1 node was forked on a chain that included the work, but the block producers on the canonical chain do not have that work in their mempool. Other nodes in the network would fail to notify the block producer's of the necessary snark work since it's already included in a fork.
So, looking at that, it seems to me that choosing to rebroadcast snark work based off of txns that need to be proven in the current canonical chain makes more sense than the old behavior. But I'm not 100% confident in this, and am curious to see what @mrmr1993 thinks.
|
Discussed with @mrmr1993 some more- with the inclusion table we were essentially only rebroadcasting work from the latest scan state (if they were no re-orgs). But we do want to rebroadcast work from scan state of older blocks given there are many tiny forks. But 10 blocks seems unnecessary and will go over the current rate limit. So now, rebroadcasting work from the scan state of last 3 blocks in the best chain. This should help with the rate limiting problem cc @nholland94 |
There was a problem hiding this comment.
After talking OOB, I'm convinced that it's better to rebroadcast older snark work from the last 3 blocks, and I'm happy approving this in its current version. This should be an improvement in all aspects of our snark pool behaviour.
There are still issues around rate limiting, but fixing those fully could end up a significant undertaking, and bitswap will fix these issues properly anyway.
limit local snark work broadcasts as per the current rate limit to avoid being rejected by peers