[chore] sidecar send new block updates #1409
Conversation
Jiloc
changed the base branch from
feat/emily-client-python
to
feat/emily-new-block-handler
| @@ -30,7 +30,7 @@ | |||
| "Tuple": { | |||
| "data_map": { | |||
| "request-id": { | |||
| "UInt": 2 | |||
| "UInt": 1 | |||
There was a problem hiding this comment.
This was needed to make the test_withdrawal_reject integration tests. Where a withdrawal can be rejected only if a connected withdrawal already exists
There was a problem hiding this comment.
Changing the test is fine, but what do we do if Emily does not have a create-withdrawal event in the database and it receives either an accept or a reject? Shouldn't it just store the event?
There was a problem hiding this comment.
At the moment, Emily returns an error when the update_withdrawal is called, and the sidecar retries indefinitely. Emily cannot store the event because withdrawals are stored as objects within a list in the corresponding Withdrawal entry in DynamoDB. If the Withdrawal entry doesn’t exist, we have no place to store its related update.
By design, this situation shouldn't happen—an update should never arrive before the withdrawal itself unless the entry was forcefully removed from the DB. Ideally, we could create the withdrawal entry when receiving an update, but the event doesn't have enough information.
I was considering a follow-up PR to introduce a new query for withdrawal lookups by [req_id, txid] (which isn't currently there). I would modify both handle_withdrawal_accept and handle_withdrawal_reject to check if the corresponding withdrawal exists before proceeding. If it doesn’t, they would return an error to the new_block handler, which would log the issue and skip processing the request, preventing unnecessary retries and potential stalls. (Same behavior as in handle_completed_deposits )
There was a problem hiding this comment.
Hmmm, I think that we can prevent it from happening. The three situations that came to mind are:
- Some bug causes the code to bail before writing to the database.
- Some error in AWS prevents writing to the database.
- The side-car being down and missing the event
initiate-withdrawal-requestevent.
They all seem unlikely for one reason or another, but I was wondering if there is some way that we can handle this more gracefully. One potential alternative to the current design is to write all events in some sort of history table. We would then potentially recreate the current state by looking at the relevant events at request time. I'm not sure how to handle the fact that the update event doesn't have enough data, although it seems better than missing even more data by dropping the event.
This kind of design would work for both deposits and withdrawals, and gets us out of having to delete or miss data, which is would save us from some headaches. The migration from our current design to the new one might be annoying but we could have both systems in place while we iron out some potential issues. What do you think?
Jiloc
changed the title
djordon
modified the milestones:
sBTC: Withdrawal Code Complete,
sBTC: Withdrawal fine tuning
| fastapi==0.115.8 | ||
| uvicorn==0.34.0 |
| @@ -30,7 +30,7 @@ | |||
| "Tuple": { | |||
| "data_map": { | |||
| "request-id": { | |||
| "UInt": 2 | |||
| "UInt": 1 | |||
There was a problem hiding this comment.
Changing the test is fine, but what do we do if Emily does not have a create-withdrawal event in the database and it receives either an accept or a reject? Shouldn't it just store the event?
| EXPOSE 20540 | ||
| ENV EMILY_API_KEY=testApiKey | ||
| ENV EMILY_CHAINSTATE_URL=http://emily-server:3031/chainstate | ||
| ENV DEPLOYER_ADDRESS=SN3R84XZYA63QS28932XQF3G1J8R9PC3W76P9CSQS |
There was a problem hiding this comment.
Ahh we don't need the deployer in the side-car since filtering is getting done in Emily itself.
There was a problem hiding this comment.
exactly, now the sidecar does the bare minimum
| class NewBlockEventModel(BaseModel, extra="allow"): | ||
| block_height: int | ||
| index_block_hash: str |
There was a problem hiding this comment.
Oh nice, that extra field is critical because we expect more fields to be sent in event from the stacks node but pydantic ignores them by default. We should probably add a comment above here describing why it's necessary.
There was a problem hiding this comment.
addressed in 658aa45
emily_sidecar/main.py
Outdated
|
|
||
| @app.get("/") | ||
| def read_root() -> dict: | ||
| return {"message": "Hello, World!"} |
There was a problem hiding this comment.
We should probably just return 200 Ok here.
## Description Part of #1067 Following #1409, the deposits and withdrawal updates will be sent to Emily only from the sidecar. In this PR we remove all calls to Emily from the new-block handler, that will now be only used to store the events in the DB. Currently, the only update that the signers send to Emily is the Deposit Accepted. We should probably consider whether we want to remove it as well. ## Changes - Remove Emily calls from signer's new_block - Adapt sbtc event handlers to only store the data and log the event. ## Testing Information - Simplify new block unit tests - Remove the stacks_events_observer integration tests ## Checklist: - [x] I have performed a self-review of my code - [x] My changes generate no new warnings - [x] New and existing unit tests pass locally with my changes - [x] Any dependent changes have been merged and published in downstream modules
Description
Part of #1067
Changes
Testing Information
make integration-testChecklist: