[v2] Read blob version params from chain #922
Conversation
|
|
ian-shim
force-pushed
the
blob-version-params
branch
4 times, most recently
from
b9796f9 to
132150e
Compare
ian-shim
force-pushed
the
blob-version-params
branch
2 times, most recently
from
6b0eba4 to
a888892
Compare
| @@ -0,0 +1,26 @@ | |||
| package common | |||
|
|
|||
| type ReadOnlyMap[K comparable, V any] struct { | |||
There was a problem hiding this comment.
ah I'd wish golang has const which will make it self-explain and enforced
node/node.go
Outdated
|
|
||
| // BlobVersionParams is a map of blob version parameters loaded from the chain. | ||
| // It is used to determine blob parameters based on the version number. | ||
| BlobVersionParams *corev2.BlobVersionParameterMap |
There was a problem hiding this comment.
This needs mutex protection for concurrent read/write
There was a problem hiding this comment.
The underlying map is read only and this pointer to the read only map gets swapped out atomically occasionally. Do we need synchronization here?
There was a problem hiding this comment.
I think it needs to, this pointer (the address itself) may be undefined if it's not synchronized among threads (not sure how it's atomic swap if no mutex)
There was a problem hiding this comment.
I think you're right. Converted these into atomic.Pointer
node/node_v2.go
Outdated
| @@ -61,7 +65,11 @@ func (n *Node) DownloadBundles(ctx context.Context, batch *corev2.Batch, operato | |||
| relayIndex := rand.Intn(len(cert.RelayKeys)) | |||
| relayKey := cert.RelayKeys[relayIndex] | |||
| for _, quorum := range cert.BlobHeader.QuorumNumbers { | |||
| assgn, err := corev2.GetAssignment(operatorState, batch.BlobCertificates[0].BlobHeader.BlobVersion, quorum, n.Config.ID) | |||
| blobParams, ok := n.BlobVersionParams.Get(cert.BlobHeader.BlobVersion) | |||
There was a problem hiding this comment.
It looks when there is a new version upgrade, it may have an outage since different dispersers/nodes up to the different versions (and potentially cannot reach quorum threshold)?
There was a problem hiding this comment.
Yes. The blob version needs to be registered onchain well before new blobs start using the new version.
There was a problem hiding this comment.
So it's like: 1) add new version of params to smart contracts; 2) let sufficient num of operators upgrade; 3) let disperser(s) upgrade ?
There was a problem hiding this comment.
- Add new blob version onchain (multisig transaction)
- Wait for
OnchainStateRefreshInterval(practically, wait for a lot more than this duration). If operator nodes, use the default value (1h), all parts of system will be aware of the new version within this time - Let disperser & relay be aware of the blob version
- Blob with new version can be dispersed
ian-shim
force-pushed
the
blob-version-params
branch
from
a888892 to
385f7c2
Compare
| @@ -224,6 +224,13 @@ var ( | |||
| Required: false, | |||
| EnvVar: common.PrefixEnvVar(EnvVarPrefix, "ENABLE_V2"), | |||
| } | |||
| OnchainStateRefreshIntervalFlag = cli.DurationFlag{ | |||
There was a problem hiding this comment.
This may have an issue about having a single control for all onchain state refresh. It's possible we need to refresh different states (e.g. blob's store duration), and maybe at different intervals. Shall we just call this BlobParamsRefreshInterval?
There was a problem hiding this comment.
I was going to have this control onchain state that changes very infrequently, for now such as relay addresses.
Maybe we can keep this name, and if we need to add other state that needs to be refreshed at different rate, we can add separate controls for those?
There was a problem hiding this comment.
This reminds me that blob store duration isn't getting refreshed at all. It only gets read on bootup. Probably fine as they don't really change, but should we refresh those as part of this loop?
There was a problem hiding this comment.
They also change very infrequently, maybe at similar level as blob params. SG to include them.
There was a problem hiding this comment.
Will include them in a follow up PR
core/v2/assignment.go
Outdated
| } | ||
|
|
||
| ops, ok := state.Operators[quorum] | ||
| if !ok { | ||
| return nil, fmt.Errorf("no operators found for quorum %d", quorum) | ||
| } | ||
|
|
||
| if len(ops) > int(params.MaxNumOperators()) { | ||
| return nil, fmt.Errorf("too many operators for blob version %d", blobVersion) | ||
| if len(ops) > int(blobParams.MaxNumOperators) { |
| } | ||
|
|
||
| numOperators := big.NewInt(int64(len(ops))) | ||
| numChunks := big.NewInt(int64(params.NumChunks)) | ||
| numChunks := big.NewInt(int64(blobParams.NumChunks)) |
There was a problem hiding this comment.
This is to convert uint64 to bigint. Do you mean use uint64 to represent numChunks instead of bigint?
| @@ -58,9 +56,9 @@ func GetAssignments(state *core.OperatorState, blobVersion BlobVersion, quorum u | |||
| mp += int(a.chunks) | |||
| } | |||
|
|
|||
| delta := int(params.NumChunks) - mp | |||
| delta := int(blobParams.NumChunks) - mp | |||
There was a problem hiding this comment.
Same, using uint instead of int.
There was a problem hiding this comment.
I think it should be int. Otherwise, delta can underflow if mp > numChunks
| } | ||
| chunks, assignment, err := v.validateBlobQuorum(quorum, blob, blobParams, state) | ||
| if errors.Is(err, ErrBlobQuorumSkip) { | ||
| continue |
| validVersions := make([]int32, 0, len(s.onchainState.BlobVersionParameters)) | ||
| for version := range s.onchainState.BlobVersionParameters { | ||
| if _, ok := onchainState.BlobVersionParameters.Get(corev2.BlobVersion(blobHeaderProto.GetVersion())); !ok { | ||
| validVersions := make([]int32, 0, onchainState.BlobVersionParameters.Len()) |
ian-shim
force-pushed
the
blob-version-params
branch
from
385f7c2 to
b678db5
Compare
Why are these changes needed?
Remove hardcoded blob version params map, and read the values from chain.
Checks