Conversation
| -- TODO: Update the metric | ||
| cancelBlocksAfterSlot :: Connection -> SlotNo -> IO () | ||
| cancelBlocksAfterSlot conn slotNo = do | ||
| let q = "UPDATE block SET status = :set_status WHERE slot_no > :slot_no;" |
There was a problem hiding this comment.
I think this is probably correct, but I think I'd need to see an argument that this is the right predicate - I think technically when we see a rollback, every block from the rollback point to the current top needs to be marked, but I'm not sure if we need a more complex query (and more metadata) to track the previous block for each block to say "all the blocks from the current tip back to the rollback point via their parent hashes needs to be cancelled".
I'm not sure if that is equivalent to "all blocks after slow N need to be cancelled", whether there's some kind of race condition were we could have inserted info about a new block after the rollback point which then gets cancelled erroneously or not. I can't actually think of how this could happen, but it feels like it may be possible, and difficult to test for.
We should have a chat about this to make sure the logic is definitely right.
There was a problem hiding this comment.
Yes, agreed. We need to discuss.
| slot_no UNSIGNED INTEGER NOT NULL | ||
| block_no UNSIGNED INTEGER, | ||
| slot_no UNSIGNED INTEGER NOT NULL, | ||
| status TEXT CHECK(status IN ('unknown', 'canceled', 'committed')) |
There was a problem hiding this comment.
Why do we need status? We already know if the block was cancelled by cancellation_event which references block.
And I don't see how we can use committed status in our app. There is no any functionality that depends on that.
There was a problem hiding this comment.
- This will make more sense once I show the UI.
- We will query all executions related to cancelled blocks and they will be used in the rollback payload.
- We need to track the status of the block somehow. The block table seems to be the most optimal place to track this.
| hash BLOB PRIMARY KEY, | ||
| block_no UNSIGNED INTEGER NOT NULL, | ||
| slot_no UNSIGNED INTEGER NOT NULL | ||
| block_no UNSIGNED INTEGER, |
There was a problem hiding this comment.
I think I would rather drop the reference constraint from cancellation_event.block_hash than NOT NULL here... This partiality of the block feels strange. While the cancellation_event can reference some block_hash we don't have.
There was a problem hiding this comment.
The reference constraint is good. I think we should have that.
This partiality of the block feels strange
I don't see this as a partial block. The identity of a block is block_hash right?
But I see where you're coming from. Let me think if there is a better way to represent this in the DB.
99625fa to
a54c86c
Compare
- Change the event intereface - Update the db structure
| target_script_hash BLOB NOT NULL, | ||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP | ||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| blocks_cancelled BLOB NOT NULL |
There was a problem hiding this comment.
CREATE TABLE IF NOT EXISTS rollback_event_blocks (
event_id INTEGER NOT NULL REFERENCES rollback_event(event_id),
block_hash BLOB NOT NULL REFERENCES block(block_hash)
)
axman6
left a comment
axman6
left a comment
There was a problem hiding this comment.
As discussed earlier, happy to merge once there's a better solution for inducing rollbacks (maybe that should be split into its own PR at this stage, since I think it might take some time to get the networking side happening)
Yeah, that's a good idea. |
3 participants
TODO: