Feature/synchronizer #5
Conversation
db/db.go
Outdated
| @@ -60,11 +61,55 @@ func (p *DB) GetOffChainData(ctx context.Context, key common.Hash, dbTx pgx.Tx) | |||
| valueStr string | |||
| ) | |||
|
|
|||
| println(key.Hex()) | |||
db/db.go
Outdated
| if err := dbTx.QueryRow(ctx, getOffchainDataSQL, key.Hex()).Scan(&valueStr); err != nil { | ||
| if errors.Is(err, pgx.ErrNoRows) { | ||
| return nil, state.ErrStateNotSynchronized | ||
| } | ||
| return nil, err | ||
| } | ||
| valueStr = strings.TrimPrefix(valueStr, "0x") // is this right? |
There was a problem hiding this comment.
no, the expected response is 0x...
There was a problem hiding this comment.
What is the right way to decode the hex? common.HexToBytes fails if the '0x' is included. maybe common.FromHex?
| Value: bytes, | ||
| } | ||
| } | ||
| return data, err |
There was a problem hiding this comment.
should double check that the answer is correct: key == hash(data)
There was a problem hiding this comment.
| ) | ||
|
|
||
| if err := db.pg.QueryRow(ctx, keyExists, key.Hex()).Scan(&count); err != nil { | ||
| return false |
There was a problem hiding this comment.
we should handle the case of error in a different way as false. For instance if there is a DB error we should retry instead of assuming that the entry is not stored there
There was a problem hiding this comment.
Retrying on DB errors can be tricky, there are classes of errors that won't resolve with retry, so can get into a loop. I guess maybe have a fixed number of retries, but then what is the behavior if you reach that?
| } | ||
|
|
||
| // wait on events, timeouts, and signals to stop | ||
| select { |
There was a problem hiding this comment.
shouldn't the select be inside an endless for loop so the subscription is re-used until there is a problem?
There was a problem hiding this comment.
synchronizer/batches.go
Outdated
| } | ||
|
|
||
| // Start starts the BatchSynchronizer event subscription | ||
| func (bs *BatchSynchronizer) Start() { |
There was a problem hiding this comment.
I'm not sure if this implementation will handle reorgs well, have no experience working with the Watch* methods
synchronizer/committee.go
Outdated
| ) | ||
|
|
||
| // DataCommitteeTracker tracks changes to the data committee membership | ||
| type DataCommitteeTracker struct { |
There was a problem hiding this comment.
If I understood well, the DAC info is only used to query data that for some reason in missing on the local DB. If this is the case, I think it's simpler and cleaner to ask for the current DAC members when needed, rather than keeping track of all the updates.
At least this is how I've implemented this on the node here 😇
The idea is that:
- You ask for the current committee only when you actually need to get data from it
- Once you've get the current committee, you cache on memory this info
- Only update if at some point all the (cached) committee members fail to response to a query
There was a problem hiding this comment.
Good suggestion, much better
…d, evict members on errors during resolve batches
This PR adds a data synchronizer.