Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cobalt: Support 2 versions of escrow events. Disable non-negativity check for escrow values. #370

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

mitjat
Copy link
Contributor

@mitjat mitjat commented Mar 30, 2023

The consensus indexer halted mid-Cobalt (block 3_028_201) because it was unable to interpret some of the events. It turns out there was a breaking change mid-Cobalt: two event types gained an additional field that was not included in indexer's frozen copy of "cobalt-compatible" types. This PR adds those fields to our copy of the types.

These additional fields were introduced in oasisprotocol/oasis-core#3944, which made it into oasis-core v21.2.0. The same consensus version (meaning it's still Cobalt) was used in 21.3, so maybe I should have based the indexer copy of Cobalt APIs on that. OTOH then there would've been no guarantee that events from 21.1 (the earliest oasis-core in Cobalt, and what indexer uses now) would be parsed successfully. Fixing this by trial and error is fine IMO.

Aside: That same PR also introduces emitting more comprehensive events for award disbursement. I don't know the award mechanism closely enough, but it looks like we can ignore the fact that semantics were different in the olden times: The events are what they are anyway, so we just log them. And the way they affect state (balances, shares) is not important because we'll override the state with Damask genesis. It would be important to support the old semantics if we wanted to support time-travel in the indexer.

Testing: Indexed cobalt, block 3_027_601 (genesis) to 3_072_768.

EDIT: After running this for longer, indexer halted at 3_230_738 with UPDATE chain.accounts SET escrow_balance_debonding = escrow_balance_debonding - $2, escrow_total_shares_debonding = escrow_total_shares_debonding - $3 WHERE address = $1 [oasis1qp60saapdcrhe5zp3c3zk52r4dcfkr2uyuc5qjxp 1513956079229 0]}: ERROR: value for domain uint_numeric violates check constraint "uint_numeric_check". Very likely, an account didn't receive the NewShares event property in one of the early blocks (due to the old version of events), but then did have its shares reduced by a new-style event at a later round. The workaround is to disable non-negativity checks for the escrow columns in the db. Once we implement parallel processing, we can re-add this check because like many other checks, we'll need to programmatically disable them before parallel processing (as out-of-order dead reckoning will violate the checks), then programmatically re-enable them afterwards.

@mitjat mitjat changed the title storage/nodeapi/cobalt: Support mid-Cobalt chang in escrow events storage/nodeapi/cobalt: Support mid-Cobalt change in escrow events Mar 30, 2023
@pro-wh
Copy link
Collaborator

pro-wh commented Mar 31, 2023

oasis-core policy is that changes to events aren't breaking, because they're local only. one thing we could do is sync a new (i.e. the last cobalt-compatible version) node from our archival copy of cobalt history and have the events be uniform throughout.

we could even backport post-cobalt changes to the events, if such mischief would be easier than guessing about data that we don't have through earlier event structures

@mitjat
Copy link
Contributor Author

mitjat commented Apr 3, 2023

oasis-core policy is that changes to events aren't breaking, because they're local only. one thing we could do is sync a new (i.e. the last cobalt-compatible version) node from our archival copy of cobalt history and have the events be uniform throughout.

We could, but we'd still need this PR (because it adds the fields that appeared in the last cobalt-compatible version of types), and the effect would be the same as only using this PR. The new fields were absent in earlier blocks, and for those blocks, the new-version node would simply assign default values to them, right? Which is the same thing that the analyzer now does.

we could even backport post-cobalt changes to the events, if such mischief would be easier than guessing about data that we don't have through earlier event structures

We're not guessing the value of AddEscrowEvent.NewShares and ReclaimEscrowEvent.Shares as much as we're ignoring it. No matter value is provided here, its potential dead-reckoning effect will be obsoleted by the Damask genesis.

@pro-wh
Copy link
Collaborator

pro-wh commented Apr 3, 2023

k

@pro-wh
Copy link
Collaborator

pro-wh commented Apr 3, 2023

The new fields were absent in earlier blocks, and for those blocks, the new-version node would simply assign default values to them, right?

the events aren't stored in the chain's source of truth. so the new node would process the blocks and transactions with the new code that emits the calculated "new shares" field

@mitjat
Copy link
Contributor Author

mitjat commented Apr 3, 2023

the events aren't stored in the chain's source of truth. so the new node would process the blocks and transactions with the new code that emits the calculated "new shares" field

I didn't know that. Thank you! For the record, you clarified more on Slack: when a block gets synced to the node, the node executes it to produce all the data it needs, including the events. Then it can answer TransactionsWithResults() with that data.

So to have events re-created in the new format, there's some hoops to jump through: We'd need to first bulk-copy the data (which is how we normally replicate archive nodes) into a non-archive node, and then have a new node connect to it and sync block by block. Still, it's doable, and might be valuable in the future.


For the purpose of this PR though, I don't think we need any of that, for the reasons described above.

Copy link
Collaborator

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya make it not crash

@mitjat mitjat force-pushed the mitjat/cobalt-richer-staking-events branch from fe95dd8 to fdba816 Compare April 3, 2023 20:20
@mitjat mitjat enabled auto-merge April 3, 2023 20:20
@mitjat mitjat force-pushed the mitjat/cobalt-richer-staking-events branch from fdba816 to 6549084 Compare April 3, 2023 20:25
@mitjat mitjat disabled auto-merge April 3, 2023 20:27
@mitjat mitjat force-pushed the mitjat/cobalt-richer-staking-events branch 2 times, most recently from 82bbc5f to d702c02 Compare April 3, 2023 22:26
@mitjat mitjat changed the title storage/nodeapi/cobalt: Support mid-Cobalt change in escrow events Cobalt: Support 2 versions of escrow events. Disable non-negativity check for escrow values. Apr 3, 2023
@mitjat
Copy link
Contributor Author

mitjat commented Apr 3, 2023

ya make it not crash

👍
I applied the type change in both staging and prod dbs manually.

@mitjat mitjat force-pushed the mitjat/cobalt-richer-staking-events branch from d702c02 to d0bb1b1 Compare April 11, 2023 14:31
@mitjat mitjat enabled auto-merge April 11, 2023 14:32
@mitjat mitjat merged commit 239a8fa into main Apr 11, 2023
@mitjat mitjat deleted the mitjat/cobalt-richer-staking-events branch April 11, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants