-
Notifications
You must be signed in to change notification settings - Fork 50
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
traversal: implement monotonically decrementing budgets. #260
Conversation
4a45537
to
e917bba
Compare
(Oh, hey, I guess this addresses protocol/web3-dev-team#27 . Cool.) |
This has unit tests, and typed errors now. @ribasushi you seemed to have opinions about what kind of info you wanted to see in errors -- can you give that bit in particular a hairy eyeball? |
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.
Left a couple nit-comments, good to ship as-is without changes.
traversal/walk_test.go
Outdated
}) | ||
qt.Check(t, order, qt.Equals, 3) | ||
qt.Assert(t, err, qt.Not(qt.Equals), nil) | ||
qt.Check(t, err.Error(), qt.Equals, `traversal budget exceeded: budget for links reached zero as we reached path "3" (link: "baguqeeyexkjwnfy")`) |
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.
@warpfork shouldn't there be a leading slash to indicate "path starts here" in path stringifications?
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.
There hasn't been anywhere in this implementation yet. 🤷
(And you and I both know we've tried to get some sort of quorum formed to have that discussion in full so we could decisively finalize something well-specified for path encoding... Alas...)
OK, so you must set a value for both budgets even if you only care about one - and most likely use-cases only care about one. I see a lot of code with max int64 for the node budget in our future. tbh I'm not super enthused about this because it's all just arbitrary numbers, but in lieu of a more intelligent approach this will have to do for now. |
It's optional, and currently not on by default, but easy to set.
Co-authored-by: Peter Rabbitson <ribasushi@protocol.ai>
059c0b3
to
3f112b4
Compare
Took the proposed wording changes. Agree it's arbitrary numbers, and that non-total functions aren't fun, but budgets are also the simplest (and thus most solid and reliable) way to ensure resources aren't being consumed beyond reason. Also true that we can probably expect a lot of code with max int64 for the node budget in the future. However, I don't really have a problem with that. It's the simplest and clearest API I can think of. (Alternative ideas seem to end up with either more bools or more pointers, and don't seem to get easier to explain or shape up more usable.) |
Sounds like we're happy enough with this, so, moving to merge. |
This patch adds budgets to traversals. Setting a budget allows you to make a traversal halt and return within a fixed number of steps.
It's optional, and currently not on by default, but easy to set.
Budgets can be set in terms of either number of nodes visited, or number of links loaded.
A few other minor target-of-opportunity improvements to internal documentation are included, while I was passing through.