-
Notifications
You must be signed in to change notification settings - Fork 54
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
A0-4346: Gather signatures and actually submit scores to chain #1907
base: main
Are you sure you want to change the base?
A0-4346: Gather signatures and actually submit scores to chain #1907
Conversation
478d4f5
to
1ab088b
Compare
1ab088b
to
e694d21
Compare
debug!(target: LOG_TARGET, "Received ABFT score: {:?}.", score); | ||
self.metrics.report_score(score[self.my_index]); | ||
// TODO(A0-4339): sometimes submit these scores to the chain. | ||
self.metrics.report_score(points[self.my_index]); |
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 guess self_my_index
cannot ever be out of points
bounds 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, it should always return points of the same length, equal to the total number of nodes. We could try handling failures there here, but I don't think it's worth it.
session_over = true; | ||
}, | ||
}, | ||
maybe_performance_hash = performance_from_scorer.next(), if !no_more_performance && !session_over => match maybe_performance_hash { |
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 here && !session_over
? What happens if we remove this?
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. If it wasn't here, it would try submitting performance that is from the previous session. This will also work, by just failing at a later point, but imo it's clearer to already stop here – if you think otherwise I can remove this check.
}; | ||
|
||
const SCORE_SUBMISSION_PERIOD: usize = 300; |
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.
Will this aleph-node
version publish some scores actually? I run it locally with below diff included, waited 300s, and nothing happened in the logs, nor in metrics
[14:55] marol-Latitude-5521:aleph-node ((e694d21b) *%) | for port in 9615 9616 9617 9618; curl localhost:9616/metrics 2>/dev/null | grep
"my_abft_score{"; end
my_abft_score{chain="a0dnet1"} 0
my_abft_score{chain="a0dnet1"} 0
my_abft_score{chain="a0dnet1"} 0
my_abft_score{chain="a0dnet1"} 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.
No because the first nonce in every session is 0 and should be 1.
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.
Also, remember that you have to specify finality version as current.
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 checked properly and it seems to be working as intended now. Still requires bumping finality version to 5, but this is by design.
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 mean bumping in primitives? There is a flag in run_nodes for that now.
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.
Just one small error, besides that good to go. Congrats for winning the uneven fight with compiler for pushing runtime API💪
Description
Send performance scores to the aggregator for gathering multisignatures under them and then submits them on chain. This should finish the ABFT performance project on the node side, modulo testing.
Type of change
Checklist: