simplified block request calls #2346
Conversation
| if !emptyBlockPermitted && !m.HasTxs() { | ||
| return | ||
| } |
There was a problem hiding this comment.
Removing this check is what I was worried about. I think the explicit override is less error-prone- we don't want to accidentally build empty blocks
There was a problem hiding this comment.
I think this is still a concern, this PR accounts for not calling it in Add when there are no txs added but this method is also consumed outside of the Mempool interface in the builder
There was a problem hiding this comment.
I don't really see where the concern is
There was a problem hiding this comment.
This PR does not guard against sending extra common.PendingTxs messages if the mempool is empty in RequestBuildBlock. Since it is an exported method on the Mempool interface, I believe it should guard against that. This is consistent to how the mempool is implemented in the AVM https://github.com/ava-labs/avalanchego/blob/dev/vms/avm/txs/mempool/mempool.go#L202-L204
With the way this PR is implemented, any consumers of the Mempool interface can call RequestBuildBlock un-restricted. When reviewing future PRs that interact with the mempool, we'll need to keep in mind that there is no guard for the P-Chain mempool. With an explicit boolean that overrides the check, reviewers can easily tell that the code is correct and does not send extra common.PendingTxs messages to the engine.
// notify engine only if we are ready to build a block
if addedTx {
m.RequestBuildBlock()
}I'd like to move forward with the base PR for these reasons (consistency + more obviously correct code). If you disagree, let's discuss offline.
There was a problem hiding this comment.
I really don't think #2333 it's more obviously correct. I think the two alternatives are equally correct.
I would not speculate on what option is easier to inspect and instead wait for other reviewers to evaluate the proposals.
| } | ||
|
|
||
| func (m *mempool) Add(tx *txs.Tx) error { | ||
| func (m *mempool) Add(txs []*txs.Tx) error { |
There was a problem hiding this comment.
This won't add any of the later txs in txs if one in the middle fails a check. I think we should retain the single tx Add instead of moving to an array
There was a problem hiding this comment.
Thanks for spotting this. Fixed while keeping the addition of a tx slice. I really like the simmetry with Remove
|
fwiw I also prefer the code as it is in https://github.com/ava-labs/avalanchego/pull/2333/files |
Why this should be merged
We could simplify the mempool api further by:
How this works
Just a mempool interface change
How this was tested
CI