Define staking events #4449
Define staking events #4449
Conversation
plaidfinch
force-pushed
the
define-staking-events
branch
from
cd04238
to
f094f22
Compare
cratelyn
added
A-staking
crates/core/component/stake/src/component/validator_handler/validator_manager.rs
Show resolved
Hide resolved
plaidfinch
force-pushed
the
define-staking-events
branch
from
f094f22
to
35624bb
Compare
plaidfinch
force-pushed
the
define-staking-events
branch
from
79e63d1
to
2556702
Compare
zbuc
force-pushed
the
define-staking-events
branch
from
2556702
to
caebf11
Compare
|
This LGTM, I made a couple code organization changes and tested it and confirmed the deferred events work properly. I have not tested it between upgrades however. |
|
Is anything blocking merging this? |
Only review |
crates/core/app/src/app/mod.rs
Outdated
| // Apply the state from `init_chain` and return the events generated during genesis. These | ||
| // cannot be emitted at this time due to a limitation of ABCI, but they will be emitted | ||
| // during the first BeginBlock. | ||
| let events = state_tx.apply().1; | ||
|
|
||
| // Commit the genesis events to the state. | ||
| let mut state_tx = self | ||
| .state | ||
| .try_begin_transaction() | ||
| .expect("state Arc should not be referenced elsewhere"); | ||
| for (i, event) in events.into_iter().enumerate() { | ||
| state_tx.put_deferred_event(i, event); | ||
| } |
There was a problem hiding this comment.
On reflection I have more concerns that this may be the wrong approach and I think we probably shouldn't do it.
One of the good things about the ABCI design is that it separates the "init chain" and "apply block" methods, so that a block is always "apply changes to a previous valid state". In contrast, Bitcoin or other chains have a notion of a "genesis block", which is a delta-update-from-an-empty state that then has special validation rules. This turns out to be problematic because all the downstream code has to have special case handling.
I worry that this design means that anything consuming ABCI events can no longer assume that an ABCI event is a delta update to an existing state. Instead, downstream consumers of events have to figure out whether or not there was a valid previous state and implement "transition from empty" logic.
Stepping back a bit, why did we want to do this at all? The problem we're trying to solve is that the ABCI events emitted as part of blocks are only recording changes to the existing state, so something that's consuming the events doesn't know what the start is. But, we're trying to fix that by changing the meaning of events, rather than by having a starting state. Instead, I think it would be a better design to assume that an indexer has access to the genesis data and can use that for its own initialization.
There was a problem hiding this comment.
Is there another way we should handle it?
There was a problem hiding this comment.
@zbuc sorry, i got distracted doing the full writeup and was delayed. filled in the comment now
There was a problem hiding this comment.
So the idea is that we remove this specially cased event emission and then let the indexers use genesis JSON to bootstrap initial state? Sounds fine to me.
There was a problem hiding this comment.
@hdevalence indexers would also need to handle the genesis data checkpointed during upgrades. In the case of an upgrade it might be preferable for an indexer to wipe their database and start from the new genesis data.
zbuc
force-pushed
the
define-staking-events
branch
from
2f9501b
to
1ac30d2
Compare
zbuc
force-pushed
the
define-staking-events
branch
from
1ac30d2
to
c8880a1
Compare
Generate code based on new staking events
…lashing penalty events
zbuc
force-pushed
the
define-staking-events
branch
from
c8880a1
to
17225d8
Compare
Describe your changes
Resolves #4336, emitting events for validator definitions and state transitions. Included events:
All of these are proto-based events, and emitted both at genesis, when relevant, and when changed later.
Emitting events at genesis is technically impossible because the ABCI interface does not accept events as a response to
InitChain, so instead genesis events are emitted at the start of the first block.Issue ticket number and link
Resolves #4336. Closes #3797. Closes #4512.
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: