fix: kli delegate confirm no longer hangs; and other delegated ixn fixes #1100
Conversation
kentbull
force-pushed
the
delegation-escrow-fixes
branch
from
2d1608c to
c0efa85
Compare
|
This PR backports the Further, the fix that |
|
Fixes #1087 |
|
Fixes #503 |
kentbull
changed the title
src/keri/core/eventing.py
Outdated
| if ilk in (Ilks.icp, Ilks.dip): # first seen and inception so verify event keys | ||
| # check to see if the dip has already been approved and if so look up delseqner and delsaider | ||
| if ilk == Ilks.dip: | ||
| delpre = ked["di"] |
There was a problem hiding this comment.
Add a check that delpre is me so that this seal lookup only runs for the delegator.
There was a problem hiding this comment.
There is different logic for each of the five roles vanilla controller, delegator controller, delegatee controller, witness, watcher.
Three controller types: delegatee controller, and non delegatee controller (vanilla), delegator. Logic for a controller for its own KEL is the same regardless of whether it is a delegate or a vanilla controller. Difference with a delegator is it has an additional process for deciding whether something gets anchored.
Witnesses don't currently care whether they are witnessing for a delegated or non-delegated identifier.
Watchers won't accept something from a delegate until it is approved.
There was a problem hiding this comment.
Pull this logic out to the delegables escrow processor that leads up to this if possible. The main Kevery.processEvent should not be doing this discovery and OOBI resolution work. Instead that is what escrows are for, to do all the processing and lookup before calling processEvent.
So, this should pop out to escrow on initial OOBI, then an escrow processing function should look up the delseqner and delsaider and then call processEvent after looking them up.
There was a problem hiding this comment.
I found out this logic is already in the escrow. I must have encountered this behavior in some intermittent way. I thought I had this reproduced, yet I must have been wrong.
src/keri/core/eventing.py
Outdated
|
|
||
| if self.locallyDelegated(delpre) and not self.locallyOwned(): # local delegator | ||
| # should only run for delegated inception and rotation, not interaction. Ixn does not require approval. | ||
| if serder.ilk in (Ilks.dip, Ilks.drt) \ |
There was a problem hiding this comment.
Have this be a not Ilks.ixn
kentbull
force-pushed
the
delegation-escrow-fixes
branch
from
c0efa85 to
7760105
Compare
…re-escrows approved dip Prior to this commit when performing OOBI resolution then any approved dip events were being re-added to the delegables escrow for another approval, which is incorrect because the dip was already approved. Adding a check during Kever.processEvent to look up the sequence number and digest of the seal fixes this. And group interaction events do not need delegation approval and thus should not be sent to the delegables escrow. This PR puts a type check for dip and drt around the logic that adds items to the delegables delegation approval escrow. There was also a third bug in the addition of dip and drt events to the delegables escrow. Only single sig events were being added to the escrow because the self.locallyDelegated() check was too restrictive and did not work for group multisig events. Adding the or self.locallyMembered(delpre) check loosened the check enough to work for group multisig events Signed-off-by: Kent Bull <[email protected]>
kentbull
force-pushed
the
delegation-escrow-fixes
branch
from
7760105 to
2c824fe
Compare
1 participant
This PR fixes the
kli delegate confirmhanging by loosening the delegation approval check to include group multisig events, specificallydipanddrtevents.Also, group ixns (interaction events) now skip unneeded approval check and OOBI resolution of a delegate no longer re-escrows already approved dip events.
Prior to this PR when performing OOBI resolution then any approved dip events were being re-added to the delegables escrow for another approval, which is incorrect because the dip was already approved. Adding a check during Kever.processEvent to look up the sequence number and digest of the seal fixes this.
And group interaction events do not need delegation approval and thus should not be sent to the delegables escrow. This PR puts a type check for dip and drt around the logic that adds items to the delegables delegation approval escrow.
There was also a third bug in the addition of dip and drt events to the delegables escrow. Only single sig events were being added to the escrow because the self.locallyDelegated() check was too restrictive and did not work for group multisig events. Adding the
Kever.locallyDelegatedimplementation check ofself.prefixesloosened the check enough to work for group multisig events.TODO: add tests to this PR.
All existing tests pass, yet the fact that this PR is needed exposes that multiple tests around multisig ixn and dip event escrowing are missing. I will add those tests to this PR so we can avoid breakages here in the future.