-
Notifications
You must be signed in to change notification settings - Fork 62
KRAM Module and Point Implementation for QRY messages #1070
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
base: main
Are you sure you want to change the base?
Conversation
…uery messages(non-trans, does not need escrow).
@@ -132,7 +133,7 @@ class Parser: | |||
|
|||
def __init__(self, ims=None, framed=True, piped=False, kvy=None, | |||
tvy=None, exc=None, rvy=None, vry=None, local=False, | |||
version=Vrsn_2_0): | |||
krm=None, version=Vrsn_2_0): |
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.
Does it make sense to integrate this at the parser level? While consistent with how we have coupled many other concerns in the Parsing class should we be separating out processing from applying message policies like this?
I wonder if we are putting too many responsibilities in the parser and whether it would make more sense to have the parser just output parsed, hydrated (instantiated as classes) CESR primitives than then go into a message processing pipeline to route them appropriately.
This is definitely doable within the parser, yet it seems like it should happen after something is returned from the parser.
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.
Full implementation plan is to have a KRAM escrow since point KRAM only works for non-trans aids. Open to discussion, just worth considering that the current parsing integration will probably be replaced with something more robust and non-interdependent.
tests/core/test_kraming.py
Outdated
# Set window parameters for the test AIDs | ||
tc.setWindowParameters(hab.pre, windowSize=1.0, driftSkew=1.0) | ||
|
||
def create_test_serder(ilk, timestamp=None, route=None, sourceAid=hab.pre, qBlock=None, routeParams=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.
Should this create_test_serder
fn be lifted out into conftest or some other fixtures location?
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.
Yes I think it probably should.
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.
Made this change
Returns: | ||
tuple: (windowSize, driftSkew) as floats in seconds | ||
""" | ||
# TODO: Implement message type as part of the window parameters |
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.
You could accomplish this with a key of (aid, messageType)
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.
That's actually how I've done it in my working copy! Haven't written tests for this feature yet.
|
||
# Serialize the tuple as JSON bytes for storage | ||
windowTuple = (windowSize, driftSkew) | ||
serialized = json.dumps(windowTuple).encode('utf-8') |
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.
Should we be serializing as JSON or as a set of CESR encoded floats?
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.
I am not opposed to changing it to fit whatever the best practice is; there other places in the codebase where we serialize this way when not handling KERI event data.
windowTuple = (windowSize, driftSkew) | ||
serialized = json.dumps(windowTuple).encode('utf-8') | ||
|
||
self.db.kram.pin(aid, [serialized]) |
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.
If you .pin()
with the keys of (aid, messageType)
for a key that would look like "aid.messageType" then you could support window parameters by message type.
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.
Yes! that is the plan, currently untested.
""" | ||
# TODO: Implement message type as part of the key (serder.ilk) | ||
sad = serder.sad | ||
sourceAid = sad.get("i", "") |
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.
If you do ilk = sad.get("t", "")
and then return (sourceAid, ilk)
then you can get support the message type.
src/keri/core/kraming.py
Outdated
|
||
if (messageTime < currentTime - driftSkew - windowSize or | ||
messageTime > currentTime + driftSkew): | ||
raise kering.ValidationError(f"Message is out of time window {serder.pretty()}") |
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.
In some places you do return False, <str>
and in other places you raise an error. It would make more sense to use one approach for everything. If you want the checkMessageTimeliness
to never throw then only ever return a True, msg
or False, msg
tuple and never throw an exception or only ever return True
and then throw exceptions for all timeliness validation failures.
And, what will be catching the errors for timeliness failures? Should it halt existing message processing at the parser level, as in should a KRAM failure bubble up to the Parser and just cause the message to be discarded? If so then there should be logging in the Parser when a KRAM failure occurs so that INFO or DEBUG level logging can expose when KRAM failures are causing messages to not be accepted by the parser.
And, if you do want to use exceptions then I suggest adding a new exception class for KRAM timeliness errors, maybe a KRAMError or KRAMTimelinessError. Something like that.
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.
Using a generic kering.ValidationError
prevents the caller of checkMessageTimeliness
from being able to have a more focused response to what, in my mind, could be a KRAM error. We can give the caller more information by using a custom error subclass if error handling is the way we want to go. It does seem more idiomatic in Python to throw errors when KRAM time window validation fails.
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.
Discussed during KERIpy Maintainers. Settled on a new error type for KRAM Errors.
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.
Made this change
# Message accepted, updated cached entry | ||
return True | ||
|
||
if messageTime == cachedTimestamp: |
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.
Is this too strict? As in, if any two messages have the same timestamp should they really be considered a replay? It would seem comparing the message digests in addition to the timestamp is also necessary to determine something is a replay.
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.
My understanding is if they have the same timestamp they should be treated as a replay by the time they are validated by the kraming module. The KRAM white paper describes the cache as monotonically ordered, which seems to imply that this is the appropriate level of strictness. Open to corrections/discussion here.
logger.info(f"Message accepted: {serder.pretty()}") | ||
except kering.ValidationError as e: | ||
logger.error(f"Invalid message: {e}") | ||
self.pruneCache() |
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.
Should this pruneCache()
call occur no matter what is in the .krms
DB? As in, should it be shifted to the left by one indentation level?
Another way of saying it is should prune cache only occur when there are items in the .krms
DB or should it always occur on each recur of the KramDoer?
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.
Implemented it this way because it seemed like unnecessary overhead to always be pruning the database. Open to discussing potential tradeoffs.
First pass at KRAM implementation