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

Fix for failing Reply End Roles escrows #738

Closed
wants to merge 2 commits into from
Closed

Fix for failing Reply End Roles escrows #738

wants to merge 2 commits into from

Conversation

rodolfomiranda
Copy link
Contributor

@rodolfomiranda rodolfomiranda commented Apr 4, 2024

Sometimes when escrowing reply end roles, a race condition causes that the updated end in the db is interpreted as an old end, and then the validation failed because the datetime of the new message has not increased. When that happens, the escrow of the reply message can not be confirmed and fails until timeouts.
In this PR I'm adding a validation to make sure that the end retrieved from the DB is actually an old end and not the same that we are trying to authorize.

@rodolfomiranda rodolfomiranda changed the title Fix for failing Reply End Roles scrows Fix for failing Reply End Roles escrows Apr 4, 2024
Copy link
Member

@pfeairheller pfeairheller left a comment

Choose a reason for hiding this comment

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

Can we get a unit test that triggers the bug being fixed?

@@ -4285,6 +4285,7 @@ def processReplyEndRole(self, *, serder, saider, route,
aid = cid # authorizing attribution id
keys = (aid, role, eid)
osaider = self.db.eans.get(keys=keys) # get old said if any
if osaider is not None and osaider.qb64b == saider.qb64b: osaider = None
Copy link
Member

Choose a reason for hiding this comment

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

2 things...

  1. please wrap if statements like this otherwise it triggers a PEP warning.
  2. We need to discuss this as I believe this breaks BADA RUN logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1- done
2- Yes, I need your careful review. I think it is not breaking bada run since it's preventing to compare the same said (old = new), so you'll be updating/authorizing the same aid-role-eid value again.

@rodolfomiranda
Copy link
Contributor Author

Can we get a unit test that triggers the bug being fixed?

I hit that problem working with a multisig in KERIA/Signify, and the problem is an escrow that never completes. Not sure if that can be part of a unit test. I'll take a look.

@pfeairheller pfeairheller deleted the branch WebOfTrust:development April 5, 2024 21:42
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.

2 participants