-
Notifications
You must be signed in to change notification settings - Fork 70
fix: kli delegate confirm no longer hangs; and other delegated ixn fixes #1100
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
Conversation
2d1608c to
c0efa85
Compare
|
This PR backports the Further, the fix that |
|
Fixes #1087 |
|
Fixes #503 |
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]>
7760105 to
2c824fe
Compare
The ked of the EXN was incorrectly being used. It should have been the ked of the original inception event (oicp). This change fixes that.
Sam recommended using the positive rather than the negative case for this if block to avoid trigerring it for non delegated events like icp and rot.
94f89e8 to
1c1c71e
Compare
| yield self.tock | ||
|
|
||
| print(f"Delegate {eserder.pre} {typ} event committed.") | ||
| print(f"Delegate {typ} event {eserder.pre} committed.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print statements in this file were reordered for readability.
| print(f"Delegate {typ} event {eserder.pre} committed.") | ||
|
|
||
| self.hby.db.delegables.rem(keys=(pre, sn)) | ||
| self.hby.db.delegables.rem(keys=(pre, sn), val=edig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .rem() logic was changed here to mirror what is found in the escrow logic in core/eventing.py
| inits["toad"] = oicp.ked["bt"] | ||
| inits["wits"] = oicp.ked["b"] | ||
| inits["delpre"] = oicp.ked["di"] if "di" in ked else None | ||
| inits["delpre"] = oicp.ked["di"] if "di" in oicp.ked else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bug that prevented multisig join operations from working for delegated inception and rotation, and probably for interaction as well.`
|
|
||
| if self.locallyDelegated(delpre) and not self.locallyOwned(): # local delegator | ||
| # must be local if locallyDelegated or caught above as misfit | ||
| if serder.ilk in (Ilks.dip, Ilks.drt) and self.locallyDelegated(delpre) and not self.locallyOwned(): # local delegator of delegated event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the big fix to kli delegate confirm that solves #1087. The reason this solves #1087 is because interaction events would incorrectly pass through this block to be escrowed before the type guard if serder.ilk in (Ilks.dip, Ilks.drt) was added.
Only delegated inception and delegated rotation should be going to the delegables escrow.
292d76d to
adaf464
Compare
| with openHby(name=name, base=base, salt=salt, temp=temp, cf=cf) as hby: | ||
| if (hab := hby.habByName(name)) is None: | ||
| hab = hby.makeHab(name=name, icount=1, isith='1', ncount=1, nsith='1', **kwa) | ||
| hab = hby.makeHab(name=name, icount=1, isith='1', ncount=1, nsith='1', cf=cf, **kwa) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small config bugfix to make so temp=True works correctly in tests.
| """ | ||
| pre = pre if pre is not None else "" | ||
| return self.locallyOwned(pre=pre) | ||
| return pre in self.prefixes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the big delegation fix. self.locallyOwned(...) was too strict of a check and excluded multisig events.
pre in self.prefixes works because self.prefixes includes multisig identifiers.
… Doers - eventing.py: Check for existing delegation seal in KEL before escrowing DIP/DRT events to delegables. This prevents re-escrowing events where delegation was already approved (e.g., via OOBI resolution or query). - app_helpers.py: Refactor MultisigDelegationApprover with leader/follower logic, helper functions, and explicit escrow release via processEvent(). Refactor MultisigInceptFollower with helper functions for clarity. - test_grouping.py: Update test_multisig_delegate with proper leader/follower approver configuration and full assertions including escrow cleanup.
adaf464 to
780d8f5
Compare
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.All existing tests pass, yet this PR is needed because the existing tests around multisig ixn and dip event escrowing do not cover multisig delegation. I will add those tests to this PR so we can avoid breakages here in the future.