Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3224 +/- ##
==========================================
+ Coverage 58.98% 59.16% +0.17%
==========================================
Files 2065 2064 -1
Lines 169272 168747 -525
==========================================
- Hits 99850 99841 -9
+ Misses 60658 60180 -478
+ Partials 8764 8726 -38
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // overflow, gas limit enforcement no longer works correctly. This preserves the | ||
| // historical behavior for backward compatibility. | ||
| func (txmp *TxMempool) ReapMaxTxsBytesMaxGas(maxTxs int, maxBytes, maxGasWanted, maxGasEstimated int64) (types.Txs, int64) { | ||
| if maxTxs < 0 { |
There was a problem hiding this comment.
it's okay for maxTxs to be equal to 0?
|
|
||
| // bytes limit is a hard stop | ||
| if maxBytes > -1 && totalSize+size > maxBytes { | ||
| if totalSize+size > maxBytes { |
There was a problem hiding this comment.
Just curious: any reason the bytes and gas checks happen before append but numtx check happens after append?
| } | ||
|
|
||
| txs, totalGas := s.txMempool.ReapMaxTxsBytesMaxGas( | ||
| int(s.cfg.maxTxsPerBlock()), // nolint:gosec // config values fit into int on supported platforms. |
There was a problem hiding this comment.
nit: Should we just make the first arg of ReapMaxTxsBytesMaxGas uint64 so we don't need a conversion?
| int(s.cfg.maxTxsPerBlock()), // nolint:gosec // config values fit into int on supported platforms. | ||
| utils.Max[int64](), | ||
| int64(s.cfg.MaxGasPerBlock), // nolint:gosec // config values stay within int64 range. | ||
| int64(s.cfg.MaxGasPerBlock), // nolint:gosec // config values stay within int64 range. |
There was a problem hiding this comment.
So we won't have different maxGasWanted and maxGasEstimated in the future?
|
|
||
| txs, totalGas := s.txMempool.ReapMaxTxsBytesMaxGas( | ||
| int(s.cfg.maxTxsPerBlock()), // nolint:gosec // config values fit into int on supported platforms. | ||
| utils.Max[int64](), |
There was a problem hiding this comment.
We probably should have maxBytes as well?
| Txs: txs, | ||
| // TODO: ReapMaxTxsBytesMaxGas does not handle corner cases correctly rn, which actually | ||
| // can produce negative total gas. Fixing it right away might be backward incompatible afaict, | ||
| // so we leave it as is for now. |
There was a problem hiding this comment.
Should I be worried because I see:
TotalGas: uint64(totalGas), // nolint:gosec // guaranteed to be positive
in the next line?
| _ service.Service = (*Reactor)(nil) | ||
| ) | ||
|
|
||
| const MempoolChannel p2p.ChannelID = 0x30 |
There was a problem hiding this comment.
Just curious, how was 0x30 decided?
| int64(s.cfg.MaxGasPerBlock), // nolint:gosec // config values stay within int64 range. | ||
| int64(s.cfg.MaxGasPerBlock), // nolint:gosec // config values stay within int64 range. | ||
| ) | ||
| s.txMempool.RemoveTxs(txs) |
There was a problem hiding this comment.
Just an observation: I think it's okay here to acquire mutex twice, since if the tx disappeared in between RemoveTxs still work.
But maybe some day we should re-think the mutex use in mempool, it looks correct and efficient to me in the current context, but if we add more and more features then it might be harder to reason.
| node.rpcEnv.Router = router | ||
| node.shutdownOps = makeCloser(closers) | ||
|
|
||
| if !gigaEnabled { |
There was a problem hiding this comment.
nit: I think it might help readability to add comment here why we disable the reactor for Autobahn.
| int64(n), // nolint:gosec // autobahn block numbers fit in int64. | ||
| blockTxs, | ||
| resp.TxResults, | ||
| mempool.NopTxConstraintsFetcher, |
There was a problem hiding this comment.
Does this mean we don't set constraints for tx any more?
2 participants
Also