Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
POC Signature aggregation API #368
POC Signature aggregation API #368
Changes from all commits
5a4b307
2fbaa7a
3ea7b58
09083e7
af458cf
58a0747
7ead3ab
eb90426
b6231a5
90252ef
138316c
3ea861b
a0a6fb8
d6c0aae
2356886
055e983
16be2c7
f7097ee
80f7f57
120b9ce
b1690ef
c61239e
22d6380
68b0ee2
05b1d59
079ff03
90f4005
97b926c
e90f8d2
da09584
15045f0
cc28367
9ece3be
9519013
3d970e6
2cc9280
bf9201d
f12dcea
82ef662
ea3ade8
15029ba
44ff6ed
7dcb2f0
3b01ef6
bbe3032
6802470
96e8262
73089d4
c73cf66
f8130ce
6f971c3
eadb326
79dca35
8b12597
1477133
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 the config nee to be passed by reference
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.
Not strictly no, since it gets used as interface but from what I've seen we normally pass configs as references and I've done that in the past as well. Not a big deal in general but in applications where configs get passed around a lot doing this can save on allocs and garbage collection.
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.
can we remove the pass by ref here, want to avoid it being unintentionally altered
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.
FWIW configs are being passed by reference everywhere in this codebase. As mentioned in previous comment we can clean this up but I'd like to punt on this until later
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.
same as above, good on punting but can you create an issue to track and link here
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.
Updated: #409
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.
We may want to wait until it's included in a release, but we can remove this now that ava-labs/avalanchego#3258 is merged right?
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.
Yeah we definitely need to keep it for now and I want to save bumping different versions of all our dependencies for a later PR. I am sort of considering keeping it for now as well though since there are other differences/places where
trackedSubnets
are used in the networking code.Specifically uptime tracking via ping messages. Not having
trackedSubnets
set makes peer nodes spew more logs about uptime tracking. This uptime tracking via ping messages should go away soon as well but it hasn't yet.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.
Sounds good. A more general comment, there's a lot of miscellania that's rightly being deferred in this PR. Do you mind enumerating them in a followup ticket so that they don't get lost in the chaff?
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.
Yup I will enter those issues and link them in the description for easy reference before we are ok to merge this one.
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.
do we need to watch the error returned from Dispatch?
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 think that
RecoverAndPanic
should cover our needs here but could be wrongThere 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.
do we need to set
allower
here? Or can we just set as the noop allowerThere 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 open to either. In general I like to keep these wrapper interface methods as close as possible in the signature to the original but this is already broken here since we use the legacy VDR set instead of the new SendConfig. I'm fine with either:
1️⃣ assuming our usage, since we only call send from here and do the conversion to SendConfig here and explicitly set the noopAllower here as well
2️⃣ keep the interface of Send same as network.Send and do the conversion and allower setting in the application layer.
I will defer on decision here to whatever the majority preference is