-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[PeerDAS] fixes and tests for gossiping out data columns #14102
Conversation
beacon-chain/p2p/monitoring.go
Outdated
Name: "p2p_blob_sidecar_committee_broadcasts", | ||
Help: "The number of blob sidecar messages that were broadcast with no peer on.", | ||
Name: "p2p_blob_sidecar_broadcasts", | ||
Help: "The number of blob sidecar messages that were broadcasted with no peer on.", |
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 is multiple issues with the code currently in the peerDAS
(and develop
) branches:
- The word
committee
has nothing to do here, as you fixed. (Changing this name is a breaking change, as it will possibly break some monitoring or alerting. So we'll need to add a dedicated comment in the next release note. But that's OK to change it.) - The behaviours of these metrics is strange. The expected behaviour is:
- For
p2p_blob_sidecar_attempted_broadcasts
(and its equivalent for data column): To be increased by 1 at every attempt (successful or not) - For
p2p_blob_sidecar_broadcasts
(and its equivalent for data column): To be increase by 1 at every successful broadcast.
- For
Helps need to be updated accordingly.
If you wish to apply these changes in this PR it would be great. Else, I'll do it in an other PR.
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.
It'd be better to separate this and upstream it to develop
, no reason to let it stay in PeerDAS and wait for it to be merged in one giant step (which would definitely complicate things)
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.
As for this current PR, I'll revert changes related to blob metrics
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.
Nice, thanks.
Also, I just had to rebase the peerDAS
branch on top of develop
. It messed up your branches, sorry for that.
An easy way to fix that is to:
- Reset your branches on
peerDAS
- Cherry pick the commit you added
- Push force
For information, we decided first to create a long running peerDAS
branch because peerDAS was not intended at all to be part of pectra, and we wanted to have something quickly working for the interop last month. (The peerDAS
branch was not intended to be merged into develop
.
It turned out that teams made some good progresses on peerDAS, so peerDAS is now possibly going into pectra. (And so we have to clean our long running peerDAS
branch before merging into develop
.)
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.
Rebased, wondering if there's any channels that we can DM instead of talking over github?
For context, I'm from Base team and we're more than thrilled to have PeerDAS included in Pectra as this increases blob capability for everybody and certainly for us that relieves concerns about our near term scaling ceilings on DA.
We're actively contributing in various aspects to try to make PeerDAS as mature as possible for inclusion. Would love to connect and collaborate more closely
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.
Sure. I just created a peerDAS
thread in the <>-code-contributions
channel in the DEV TALK
group of the Prysm discord.
https://discord.com/channels/476244492043812875/1250722678097186858
ca88c59
to
9a714aa
Compare
9a714aa
to
2065304
Compare
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
* [PeerDAS] Minor fixes and tests for gossiping out data columns * Fix metrics
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Minor fixes and tests for gossiping out data columns
Which issues(s) does this PR fix?
Fixes #
Other notes for review