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

debonding_delegations: remove id from the table #712

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Jun 21, 2024

Fix handling of multiple debonding-delegations ending in the same epoch.

Scenario:
A person undelegates twice from the same delegator, within the same epoch, first X, then Y shares. This ends up in two DebondingStartEscrowEvent events with the same DebondEndTime.

Note that this is likely a common scenario since people tend to first issue a small reclaim escrow as a test, and then soon after a larger reclaim escrow for the rest of the shares.

Oasis-node handling:
Oasis-node internally merges this case into a single debonding-delegation, therefore once the stake is debondend, only a single ReclaimEscrowEvent event is returned from oasis-node, with the sum of shares X+Y.

Nexus handling:
Nexus inserts two debonding delegatinos into the DB (uses a "virtual" id field to avoid conflicts) and expects two ReclaimEscrowEvents one for X shares and one for Y shares. Since these events are not receieved the debonding delegations do not get cleaned up from the nexus db.

Solution:
Remove the "id" field from chain.debonding_delegation and handle such debonding delegations ending in the same epoch by merging them.


Related mini-fix: There is an additional issue where some debonidng delegations with debond_end=0 are present in the DB:
https://nexus.stg.oasis.io/v1/consensus/accounts/oasis1qpqz2ut6a6prfcjm64xnpnjhsnqny6jqfyav829v/debonding_delegations_to
I believe these might be there because in very past (pre-damask) DebondingStartEscrowEvent did not include End time (oasisprotocol/oasis-core#4437). I did not confirm this as a fact, but this seems the most likely explanation and the only reasonable i could think of. So as a mini fix we also handle that case in ConsensusDeleteDebondingDelegations.

This issue might not be as problematic in practice, because fast_sync-ing over old blocks mitigates this.


I think these two issues explain most of the records that are wrongly returned here:
https://nexus.oasis.io/v1/consensus/accounts/oasis1qpqz2ut6a6prfcjm64xnpnjhsnqny6jqfyav829v/debonding_delegations_to

cases with duplicate debond_end are explained with the first scenario, and cases with debond_end=0 the second scenario.

However there remain a couple of records that were also not cleaned up and are not duplicated, such as:
These do not have duplicate debond_end which would be explained by the first scenario, so it's not obvious to me why these were not cleaned up:

{"amount":"3307282783472","debond_end":32577,"delegator":"oasis1qrk6htgerp3jvmql2tqhn0qfu634p6nfrs0026nf","shares":"3307282783472","validator":"oasis1qpqz2ut6a6prfcjm64xnpnjhsnqny6jqfyav829v"}
{"amount":"976804770947","debond_end":32616,"delegator":"oasis1qrh4vw8wkejftgu8kkjptl7znw0jzzxmzyw7x0g3","shares":"976804770947","validator":"oasis1qpqz2ut6a6prfcjm64xnpnjhsnqny6jqfyav829v"}
{"amount":"819431948445","debond_end":32619,"delegator":"oasis1qr2mjhexkpzjneus6adgjql57n4aqvkvvg3npu8t","shares":"819431948445","validator":"oasis1qpqz2ut6a6prfcjm64xnpnjhsnqny6jqfyav829v"}
{"amount":"2083760438599","debond_end":32710,"delegator":"oasis1qpy27lymj6pyja0hsk6fc5qzqa6ttlvr6q0wrdxt","shares":"2083760438599","validator":"oasis1qpqz2ut6a6prfcjm64xnpnjhsnqny6jqfyav829v"}
{"amount":"54527911396614","debond_end":32722,"delegator":"oasis1qzrd70su4u63ftga5vvgmqkmkd2aehky8yle2474","shares":"54527911396614","validator":"oasis1qpqz2ut6a6prfcjm64xnpnjhsnqny6jqfyav829v"}

^ I have checked a specific example from the above:

  • the db state looks correct (correct shares, debond_end)
  • i also see the staking.escrow.reclaim event in the db which should have deleted the debonding_delegation (addresses and share numbers match) - there's no duplicate dobnding_delegation here
  • so i don't see how it wouldn't be deleted, unless somehow non-matching epoch was provided in the data.Epoch
  • also there's no such cases on the testnet (all the non-cleaned debondings are duplicates there)
  • ^ maybe there was an issue during indexing/with the cache/or something during the indexing. Let's reinvestigate this in future if it appears again.
  • interestingly both staging and production have similar issues but with different records, so it looks like it's not really deterministic?

@ptrus ptrus force-pushed the ptrus/fix/debonding-delegations-id branch 3 times, most recently from cde84d2 to 3234a4c Compare June 21, 2024 11:27
delegator = $1 AND delegatee = $2 AND shares = $3 AND debond_end IN ($4::bigint, $4::bigint - 1)
LIMIT 1
)`
WHERE delegator = $1 AND delegatee = $2 AND shares = $3 AND debond_end IN ($4::bigint, $4::bigint - 1, 0)`
Copy link
Member Author

Choose a reason for hiding this comment

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

We could also do debond_end <= $4, but IMO let's keep it as is, so that any additional issues get noticed

@ptrus ptrus force-pushed the ptrus/fix/debonding-delegations-id branch 3 times, most recently from 8d75a47 to dc85fbb Compare June 28, 2024 13:26
@ptrus
Copy link
Member Author

ptrus commented Jul 1, 2024

Please review when you have some time @Andrew7234 @pro-wh @jberci

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.

I'm okay with this proposed behavior. changes look ok. I'm assuming the DO $$ / DECLARE ... work as one would guess

alternative is delete AND shares = $3 and LIMIT 1 so that we process all synthetic debonding delegations together. possibly assert that the sum is equal to the event shares.

did anyone in the explorer team say anything about wanting these to stay separate?

ORDER BY id DESC
LIMIT 1;

-- Step 3: Insert data from the old table into the new table, merging shares on conflict
Copy link
Collaborator

Choose a reason for hiding this comment

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

other steps unnumbered?

@ptrus
Copy link
Member Author

ptrus commented Jul 2, 2024

did anyone in the explorer team say anything about wanting these to stay separate?

No, and I also don't see what the benefit would be. I believe it would be confusing since it doesn't match the consensus state where these are merged.

alternative is delete AND shares = $3 and LIMIT 1 so that we process all synthetic debonding delegations together. possibly assert that the sum is equal to the event shares.

👍

@ptrus ptrus force-pushed the ptrus/fix/debonding-delegations-id branch from dc85fbb to 0cec5af Compare July 2, 2024 08:45
@ptrus ptrus force-pushed the ptrus/fix/debonding-delegations-id branch from 0cec5af to fe9a65b Compare July 2, 2024 08:47
@ptrus ptrus merged commit 8f899e6 into main Jul 2, 2024
16 checks passed
@ptrus ptrus deleted the ptrus/fix/debonding-delegations-id branch July 2, 2024 08:53
@csillag csillag mentioned this pull request Dec 22, 2024
2 tasks
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.

None yet

2 participants