Platformvm network refactor #1363
Conversation
This reverts commit f12340f.
ceyonur
added
vm
ceyonur
requested review from
StephenButtolph,
abi87,
coffeeavax and
danlaine
as code owners
| Uptimes uptime.Manager | ||
| Rewards reward.Calculator | ||
| Bootstrapped *utils.Atomic[bool] | ||
| Mempool mempool.Mempool |
There was a problem hiding this comment.
I don't think we need mempool to be an attribute of the backend. Just drop it and simply pass it in blockbuilder.New
There was a problem hiding this comment.
We already both use mempool + txExecutorBackend as arguments in blockbuilder.New and blockexecutor.NewManager. I thought Backend serves as provider for both of these New functions, so that we can use Backend to group common objects. (instead of passing it as separate argument)
There was a problem hiding this comment.
I see but the backend is used in other places where mempool is not needed. Not sure if I am changing my mind here, but now it feels cleaner to keep mempool our of backend.
Anyhow, we can let other reviewers express their opinion on this
| env.ctx.Lock.Lock() | ||
| defer func() { | ||
| require.NoError(shutdownEnvironment(env)) | ||
| env.ctx.Lock.Unlock() |
There was a problem hiding this comment.
nit: Unlock follow Shutdown call in prod code. I would keep that order here
There was a problem hiding this comment.
Hmm I was thinking that Shutdown might try to grab the lock
There was a problem hiding this comment.
No in prod code it's the message handler responsible to grab the lock. In general we don't grap the Ctx.Lock in the platformvm. It's assume that callers duly do that
|
|
||
| require.Equal(txID, retrivedTx.ID()) | ||
|
|
||
| // show that transaction is not re-gossiped is recently added to mempool |
| FlowChecker: res.utxosHandler, | ||
| Uptimes: res.uptimes, | ||
| Rewards: rewardsCalc, | ||
| Mempool: res.mempool, |
There was a problem hiding this comment.
if Mempool is removed from the backend (I think it's cleaner to pass it to block Builder from the constructor), this change can be reverted.
There was a problem hiding this comment.
I answered it above:#1363 (comment)
We would end up taking the same mempool + txExecutor args in both blockbuilder/blockexecutor.
I think we can break this dependency by making Mempool a subscribable struct then blockbuilder can subscribe to |
Co-authored-by: Vie <yangchenzhong@gmail.com>
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
Why this should be merged
We needed to separate network logic from builder to add new handlers for GossipHandler. This PR refactors the networking/handlers in platform vm by moving networking logic to a new package, and decreases any cross-dependency between platformvm-network and builder.
How this works
Moves networking logic in the builder to a separate package. Implements the new networking package with external handlers. Removes unnecessary requestIDs from
TxGossiphandlers.How this was tested
Added new UTs