-
Notifications
You must be signed in to change notification settings - Fork 74
Fix multiple cache loading message #611
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: edge
Are you sure you want to change the base?
Conversation
| case {UnsignedCommitments, SignedCommitments} of | ||
| {[], _} -> | ||
| {ok, #{ <<"commitments">> := NewCommitments }} = | ||
| {ok, #{ <<"commitments">> := NewCommitments } = LoadedMsg} = |
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.
By discarding this LoadedMsg, we discarded the loaded values from the cache. Msg value didn't contain the full values, and was used to attach merged commitments.
src/hb_message.erl
Outdated
| _ -> {undefined, #{}} | ||
| end, | ||
| {ok, #{ <<"commitments">> := NormCommitments }} = | ||
| {ok, #{ <<"commitments">> := NormCommitments } = LoadedMsg} = |
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.
Changes under the verify made sense initially, but this breaks the dev_lua and dev_lua_test tests. The verify mode is called on dev_lua:decode and do_normalize_commitments fast mode.
…fied - Add `contains_links/1` helper to detect if a message contains lazy links - Modify `do_normalize_commitments/3` to preserve the original message when it doesn't contain links, avoiding redundant loading operations - Use the original Msg instead of LoadedMsg when appropriate to maintain data integrity and prevent multiple reloads - Update test to reflect that cached values should not become links after reading (renamed to `encode_message_with_cache_values_test`) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| ?assertEqual(dev_codec_flat:from(Lifted, #{}, #{}), {ok, Map}), | ||
| ok. | ||
|
|
||
| encode_message_with_links_test() -> |
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'm not sure of the use case for this test. Is there a behaviour where we expect a message that fetches data from cache always to return a linkify version, or should we always expect the values loaded from cache to be returned downwards, so we can avoid loading them again?
| case {IsMsgLinked, IsLoadedMsgLinked} of | ||
| {true, false} -> | ||
| LoadedMsg#{ <<"commitments">> => MergedCommitments }; | ||
| _ -> | ||
| Msg#{ <<"commitments">> => MergedCommitments } | ||
| end; |
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.
Initially, I was using LoadedMsg to return instead of Msg. This works for the use case of fetching an ID, but it breaks in the hb_cache match linked message test, because now the expected value is a linkify version, but the value itself isn't.
We can see the expected value as following:
#{<<"a">>=>#{<<"b">>=><<"c">>,<<"commitments">>=>#{<<"WnTWQDw6An5hmZioVWeN1C1xvYCxyrshVPBoBZvMCUU">>=>#{<<"commitment-device">>=><<"httpsig@1.0">>,<<"committed">>=>[<<"b">>,<<"d">>],<<"keyid">>=><<"constant:ao">>,<<"signature">>=><<"WnTWQDw6An5hmZioVWeN1C1xvYCxyrshVPBoBZvMCUU">>,<<"type">>=><<"hmac-sha256">>}},<<"d">>=><<"e">>}}And the loaded value (the one returned) as:
#{<<"a+link">>=><<"WnTWQDw6An5hmZioVWeN1C1xvYCxyrshVPBoBZvMCUU">>}I'm not sure if the test is wrong and we should always return the LoadedMsg, or we should check which one is the linkify version and return the other one.
We discovered an issue where the message was fetching data from cache 3 times during a call to a transaction ID.
Example:
curl http://localhost:8734/ICC8rNh2ziKUDQHK7wnoaxtO0KNKYuW1dCwZc8B9e4gwas accessed 2 more times than usual to the cache inhb_http:replyfunction because the message handed fromdev_metatohb_http:replydidn't contain the results fetched from cache.Message was first loaded inside
do_normalize_messagecall bydev_message:commit, which callshb_message:convertand thenhb_link:normalize, which loads the information in the cache.In some cases, loading can be avoided by providing the header
bundle: false, but this wasn't a solution here.This is a list of the time spent in some function while processing the request.
Improvement
http_reply: duration: 2703(now) vshttp_reply: duration: 15207(previously).