contractcourt: batch startup reads and block epoch notifications #4697
Conversation
|
Please request a review when ready :) |
Big win either case reducing the total number of goroutines and DB transactions by such a large margin! We can possibly circle back to truly centralize the advancement of all channel arbs at a later time depending on how this PR shapes out. |
carlaKC
force-pushed
the
4481-batchstartandblocks
branch
from
0046b90 to
d298d20
Compare
carlaKC
force-pushed
the
4481-batchstartandblocks
branch
2 times, most recently
from
3ea2145 to
5c55f11
Compare
contractcourt/channel_arbitrator.go
Outdated
| // chanArbStartState starts our channel arbitrator with the state provided. | ||
| // This function should be directly called when manually starting the channel | ||
| // arbitrator with state that has been independently retrieved. | ||
| func (c *ChannelArbitrator) startWithState(state *chanArbStartState) error { |
There was a problem hiding this comment.
Can we remove the regular Start method at this point?
carlaKC
force-pushed
the
4481-batchstartandblocks
branch
from
5c55f11 to
727f69f
Compare
carlaKC
force-pushed
the
4481-batchstartandblocks
branch
3 times, most recently
from
28e6c10 to
00a5a80
Compare
There was a problem hiding this comment.
Nice work! This doesn't go quite as far as I'd envisioned re unifying all database operations across all channel arbs, but this version as is still is quite an improvement given the reduction in start up transactions as well as goroutine as a whole!
Starting to test this on one of my nodes with 300+ channels, will report back if anything odd pops up then do likely an additional round of review afterwards.
carlaKC
force-pushed
the
4481-batchstartandblocks
branch
2 times, most recently
from
0f072c8 to
e5649a0
Compare
carlaKC
force-pushed
the
4481-batchstartandblocks
branch
from
e5649a0 to
ccd49f8
Compare
To allow us to grab all of the information we need for our channel arbs in a more efficient way on startup, we add an optional tx to our lookup functions required on start.
carlaKC
force-pushed
the
4481-batchstartandblocks
branch
from
ccd49f8 to
697dbf7
Compare
Indeed! Diff since last review here with correct db resets. |
Fixes #4481:
With the change set as is, we reduce the number of database transactions we use from 2n to 1. However, if any of these channels are no longer in default state, they will still perform additional db transactions as they advance their state in
stateStep. Using a single block notification stream reduces goroutines required to deliver blocks from 2n to n.(n = number of arbitrators)
Having some grief with the itests for these, still figuring it out.
We’ve also discussed the prospect of passing a single transaction into
stateStepso that we can step all of our transactions with a single tx on block arrival and startup. Working on this for a follow up because it requires a lot of refactoring, and has some complexity becausestateSteptakes some actions that cannot be rolled back if committing our shared tx fails (eg, publishing close tnxs, launching resolvers) which could land us in strange states.-> This only gives us a performance gain at startup, not for new blocks because we only state step for default state arbs, which require no db txns.