Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix data columns sampling #14263

Merged
merged 7 commits into from
Jul 29, 2024
Merged

Fix data columns sampling #14263

merged 7 commits into from
Jul 29, 2024

Conversation

nalepae
Copy link
Contributor

@nalepae nalepae commented Jul 25, 2024 •
edited
Loading

Please read commit by commit, and please read the content of each commit message.

Some logs from Kurtosis:

[2024-07-26 12:45:55]  DEBUG sync: Start data columns sampling columns=[107, 81, 117, 40, 91, 3, 66, 69] root=0x98977d6d68b5c83eec2f8a6e2cd869d22e918cb95537bb9d54bd12578c99ac1d round=1
[2024-07-26 12:45:55]  DEBUG sync: Sampled columns from peer sucessfully peerID=16Uiu2HAmAfg7dQgGoa9E4jbDDTPkjpWJ8BSYauCUc4Cuok6mCMEF requestedColumns=[3, 40, 81, 91, 117] root=0x98977d6d68b5c83eec2f8a6e2cd869d22e918cb95537bb9d54bd12578c99ac1d
[2024-07-26 12:45:56]  DEBUG sync: Sampled columns from peer sucessfully peerID=16Uiu2HAmB8Vsg1ipYvC2R3wXBLzACJJtnYahcWTvjfoaSrZTB4kN requestedColumns=[66, 69, 107] root=0x98977d6d68b5c83eec2f8a6e2cd869d22e918cb95537bb9d54bd12578c99ac1d
[2024-07-26 12:45:56]  DEBUG sync: All columns were successfully sampled duration=0.521563s root=0x98977d6d68b5c83eec2f8a6e2cd869d22e918cb95537bb9d54bd12578c99ac1d roundsNeeded=1
  • Tested on Kurtosis with 2 Prysm super nodes & 1 Prysm full node.

nalepae added 4 commits July 25, 2024 18:04
@nalepae
Fix the obvious...
6d7df11
@nalepae
Data columns sampling: Modify logging.
2fb7721
@nalepae
waitForChainStart: Set it threadsafe - Do only wait once.
980b92e
@nalepae
Sampling: Wait for chain start before running the sampling.
7203be6 
Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.
@nalepae nalepae requested a review from a team as a code owner July 25, 2024 16:10
@nalepae nalepae requested review from kasey, rauljordan and saolyn and removed request for a team July 25, 2024 16:10
@nalepae nalepae changed the title Fix data colum sampling Fix data columns sampling Jul 25, 2024
nisdas
nisdas reviewed Jul 26, 2024
View reviewed changes
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better solution would be to put this in registerHandlers . We can change the method name to something more appropriate but if you look at the method all routines that need to wait on chain start are there

@nalepae
Copy link
Contributor Author

nalepae commented Jul 26, 2024

A better solution would be to put this in registerHandlers . We can change the method name to something more appropriate but if you look at the method all routines that need to wait on chain start are there

Fixed in 32410e3.
(Tried to find a good name for the function...)

@nalepae nalepae requested a review from nisdas July 26, 2024 17:51
nisdas
nisdas reviewed Jul 29, 2024
View reviewed changes
beacon-chain/sync/service.go Outdated
@@ -310,6 +304,10 @@ func (s *Service) initCaches() {
}

func (s *Service) waitForChainStart() {
if s.chainIsStarted() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed, we initialize some state here that is needed by our gossip handlers. If this exits early there will be panics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 46a1c6a.

@nalepae nalepae merged commit b2a6877 into peerDAS Jul 29, 2024
13 of 16 checks passed
@nalepae nalepae deleted the fix-sampling branch July 29, 2024 12:24
nalepae added a commit that referenced this pull request Jul 29, 2024
@nalepae
Fix data columns sampling (#14263)
c9fa938 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nisdas pushed a commit that referenced this pull request Aug 14, 2024
@nalepae @nisdas
Fix data columns sampling (#14263)
35f3d1f 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nisdas pushed a commit that referenced this pull request Aug 15, 2024
@nalepae @nisdas
Fix data columns sampling (#14263)
dee1751 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nisdas pushed a commit that referenced this pull request Aug 20, 2024
@nalepae @nisdas
Fix data columns sampling (#14263)
7945cb8 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Aug 27, 2024
@nalepae
Fix data columns sampling (#14263)
5b253ee 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Aug 28, 2024
@nalepae
Fix data columns sampling (#14263)
98a3652 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Aug 28, 2024
@nalepae
Fix data columns sampling (#14263)
ca7f3df 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Oct 7, 2024
@nalepae
Fix data columns sampling (#14263)
f48633d 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Oct 7, 2024
@nalepae
Fix data columns sampling (#14263)
eb88aa6 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Oct 7, 2024
@nalepae
Fix data columns sampling (#14263)
dad198f 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Oct 8, 2024
@nalepae
Fix data columns sampling (#14263)
bf08906 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Oct 23, 2024
@nalepae
Fix data columns sampling (#14263)
05540f5 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Oct 23, 2024
@nalepae
Fix data columns sampling (#14263)
95cd3a1 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Oct 24, 2024
@nalepae
Fix data columns sampling (#14263)
9ae8492 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Nov 20, 2024
@nalepae
Fix data columns sampling (#14263)
d8e73c3 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Nov 22, 2024
@nalepae
Fix data columns sampling (#14263)
cfa7c3f 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Nov 22, 2024
@nalepae
Fix data columns sampling (#14263)
474cdb6 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Nov 25, 2024
@nalepae
Fix data columns sampling (#14263)
3191810 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Nov 25, 2024
@nalepae
Fix data columns sampling (#14263)
1dc6a40 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
nalepae added a commit that referenced this pull request Nov 27, 2024
@nalepae
Fix data columns sampling (#14263)
1995431 
* Fix the obvious...

* Data columns sampling: Modify logging.

* `waitForChainStart`: Set it threadsafe - Do only wait once.

* Sampling: Wait for chain start before running the sampling.

Reason: `newDataColumnSampler1D` needs `s.ctxMap`.
`s.ctxMap` is only set when chain is started.

Previously `waitForChainStart` was only called in `s.registerHandlers`, it self called in a go-routine.

==> We had a race condition here: Sometimes `newDataColumnSampler1D` were called once `s.ctxMap` were set, sometimes not.

* Adresse Nishant's comments.

* Sampling: Improve logging.

* `waitForChainStart`: Remove `chainIsStarted` check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nisdas nisdas nisdas approved these changes

@kasey kasey Awaiting requested review from kasey kasey is a code owner automatically assigned from prysmaticlabs/core-team

@rauljordan rauljordan Awaiting requested review from rauljordan rauljordan is a code owner automatically assigned from prysmaticlabs/core-team

@saolyn saolyn Awaiting requested review from saolyn saolyn is a code owner automatically assigned from prysmaticlabs/core-team

Assignees
No one assigned
Labels
peerDAS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nalepae @nisdas