chore: put celestia features into 0.38.x by tac0turtle · Pull Request #1672 · celestiaorg/celestia-core

tac0turtle · 2025-03-12T12:31:09Z

Description

This Pr took the diff files from 0.34.x -> 0.34.x-celestia and applied the changes on top of 0.38.x branch of comet.

This contains 95% of the changes.

Missing Features that will be PRed into this branch

cat mempool
priority mempool

chore: correctly validate share proof

evan-forbes

[blocking]
where does the app mempool live now? do we just need to move cat and v1 to app? how does that work with the multiplexer?

I removed most of the changes to the e2e test in #1609 , and it simplified things.

overall I think we're good to merge, had a few minor blocking questions/comments. its difficult to ensure that everything was added, but I didn't see anything that shouldn't have been added. If there are things missing, we'll find them when we sync or connect to node and can handle them quickly then.

evan-forbes · 2025-03-12T22:12:13Z

+	// QueueUnprocessedEnvelope is called by the switch when an unprocessed
+	// envelope is received. Unprocessed envelopes are immediately buffered in a
+	// queue to avoid blocking. Incoming messages are then passed to a
+	// processing function. The default processing function unmarshals the
+	// messages in the order the sender sent them and then calls Receive on the
+	// reactor. The queue size and the processing function can be changed via
+	// passing options to the base reactor.
+	// 	QueueUnprocessedEnvelope(e UnprocessedEnvelope)


leaving a comment to link piece of code and PR to an issue. we need to handle this at some point. either remove it or re-add it

+1

There is a lot of commented code in here. We could either remove the commented code and create an issue, or uncomment it.

If we decide to remove it, then we should remove all references to the parallel processing including the queue sizes, etc

evan-forbes · 2025-03-12T22:16:16Z

+				stateMachineValid, err = bcR.blockExec.ProcessProposal(first, state)
+				if !stateMachineValid {
+					err = fmt.Errorf("application has rejected syncing block (%X) at height %d", first.Hash(), first.Height)


evan-forbes · 2025-03-12T22:18:18Z


 // NewReactor returns a new Reactor with the given config and mempool.
-func NewReactor(config *cfg.MempoolConfig, mempool *CListMempool) *Reactor {
+func NewReactor(config *cfg.MempoolConfig, mempool *CListMempool, traceClient trace.Tracer) *Reactor {


[not blocking]

I guess it doesn't hurt to add the tracer here, but we don't have any plans to trace this mempool, and we aren't adding any traces here afaict so we could also remove it

evan-forbes · 2025-03-12T22:21:21Z

 # We use Debian instead of Alpine, so that we can use binary database packages
 # instead of spending time compiling them.
-FROM cometbft/cometbft-db-testing:v0.14.2
+FROM cometbft/cometbft-db-testing:latest


I had to switch to the standard golang one to get this to work. #1609

is this a reason to use latest instead of the exact version? With latest, we might not be aware when there is a new version that breaks the tests

rach-id · 2025-03-14T11:05:03Z

 }
 func (br *ByzantineReactor) InitPeer(peer p2p.Peer) p2p.Peer { return peer }
+
+// func (br *ByzantineReactor) QueueUnprocessedEnvelope(e p2p.UnprocessedEnvelope) {


why was this commented?

ill remove this, it was for testing but then wasnt sure if i should try to add it back. there was an issue with some tests that i couldnt figure out the issue

rach-id · 2025-03-14T11:25:00Z

+	// QueueUnprocessedEnvelope is called by the switch when an unprocessed
+	// envelope is received. Unprocessed envelopes are immediately buffered in a
+	// queue to avoid blocking. Incoming messages are then passed to a
+	// processing function. The default processing function unmarshals the
+	// messages in the order the sender sent them and then calls Receive on the
+	// reactor. The queue size and the processing function can be changed via
+	// passing options to the base reactor.
+	// 	QueueUnprocessedEnvelope(e UnprocessedEnvelope)


+1

There is a lot of commented code in here. We could either remove the commented code and create an issue, or uncomment it.

If we decide to remove it, then we should remove all references to the parallel processing including the queue sizes, etc

rach-id · 2025-03-14T11:40:39Z

 # We use Debian instead of Alpine, so that we can use binary database packages
 # instead of spending time compiling them.
-FROM cometbft/cometbft-db-testing:v0.14.2
+FROM cometbft/cometbft-db-testing:latest


is this a reason to use latest instead of the exact version? With latest, we might not be aware when there is a new version that breaks the tests

damiannolan

quick question

) This pr extends abci Checktx in order to facilitate priority ordering in core for p2p dissemination.

rach-id · 2025-03-26T11:45:56Z

Still looking good to me 👍 👍

evan-forbes · 2025-03-26T16:34:08Z

I think we're just waiting on the e2e test @tac0turtle

it looks like a silly failure, happy to fix this to help push this through if needed

tac0turtle · 2025-03-26T17:06:58Z

I think we're just waiting on the e2e test @tac0turtle

it looks like a silly failure, happy to fix this to help push this through if needed

i havent had a moment to properly debug, but the flag is there so not sure what is missing.

tac0turtle · 2025-03-27T12:13:12Z

e2e is fixed and made the tests work with pebble as well

this pr adds the header to finalize block as its stored in state. since abci++ does not contain the whole header it was breaking sync of v3 --------- Co-authored-by: Marko Baricevic <markobaricevic@Markos-MacBook-Pro.local>

damiannolan · 2025-03-28T09:44:46Z

+	s.server = grpc.NewServer(
+		grpc.MaxRecvMsgSize(math.MaxInt32),
+		grpc.MaxSendMsgSize(math.MaxInt32),
+	)


do these need to be set to MaxInt32 or can we use some const like (max block size + some delta)

send is already int32, i set the main version to 75, i can do it here.

damiannolan · 2025-03-28T09:46:49Z

 	handshaker := NewHandshaker(stateStore, state, blockStore, gdoc)
 	handshaker.SetEventBus(eventBus)
-	err = handshaker.Handshake(proxyApp)
+	_, err = handshaker.Handshake(proxyApp)


what is the ignored arg? 🤔

the version in the responseInfo

rach-id · 2025-03-31T17:08:55Z

 // Copied unchanged from reactor.go so the correct respondToPeer is called.
-func (bcR *ByzantineReactor) Receive(e p2p.Envelope) { //nolint: dupl
+func (bcR *ByzantineReactor) Receive(e p2p.Envelope) {
+	fmt.Println("Receive", e.Message)


looks to be a residual print

rach-id · 2025-03-31T17:14:38Z

+func (evR *Reactor) OnStop() {
+	evR.BaseReactor.OnStop()
+}


this feels redundant since OnStop() is already called automatically when stopping a reactor

tac0turtle and others added 21 commits February 10, 2025 15:31

modify and generate protos

2aabfed

more changes

12a6889

builds

9cb80b9

fix test

13de6d5

updates

62954f2

fix rpc tests

6f519f5

fix tx.go

7cc8caf

fix linting issues

89351c2

fix proto linting

d3c18d7

go mod tidy

414dfb8

go vulncheck

5fdf460

weird

7639654

fix one test

f2973d9

fix mempool test

3ddbc2f

bump go version in docker

2df1c7a

test

73bd427

linter

af8e39e

remove queued messages

295d291

rename hash

16a311b

chore: correctly validate share proof

6cc2c88

Merge pull request #12 from 01builders/cian/fix-share-proof-validation

a2b8ce5

chore: correctly validate share proof

tac0turtle changed the title ~~Marko/core changes v3~~ chore: put celestia features into 0.38.x Mar 12, 2025

evan-forbes requested review from evan-forbes, rach-id and tzdybal March 12, 2025 21:41

evan-forbes reviewed Mar 12, 2025

View reviewed changes

rach-id reviewed Mar 14, 2025

View reviewed changes

damiannolan reviewed Mar 17, 2025

View reviewed changes

Comment thread types/block.go Outdated

tac0turtle and others added 2 commits March 17, 2025 12:56

feature(abci): extend checktx API in order to facilitate ordering (#1674

41a6622

) This pr extends abci Checktx in order to facilitate priority ordering in core for p2p dissemination.

address some comments

b8db9f9

Marko Baricevic added 12 commits March 27, 2025 11:36

address comments

4f649ba

remove trace flag checks

6aecf4c

attempt to fix metrics

e952322

attempt to fix metrics

a2506eb

test

d0807ca

++

60fe7d0

++

1b4c835

++

11cde16

++

a31ec2a

++

0ca3c4b

fix e2e and setup pebbledb usage in tests

09a3105

build flag for pebble

876ab88

tac0turtle requested review from cmwaters, damiannolan and rootulp March 27, 2025 14:13

fix: add header for v3 sync (#1692)

69ddda7

this pr adds the header to finalize block as its stored in state. since abci++ does not contain the whole header it was breaking sync of v3 --------- Co-authored-by: Marko Baricevic <markobaricevic@Markos-MacBook-Pro.local>

rootulp previously approved these changes Mar 27, 2025

View reviewed changes

evan-forbes previously approved these changes Mar 28, 2025

View reviewed changes

damiannolan reviewed Mar 28, 2025

View reviewed changes

reduce size

7335c6c

tac0turtle dismissed stale reviews from evan-forbes and rootulp via 7335c6c March 28, 2025 14:25

evan-forbes approved these changes Mar 31, 2025

View reviewed changes

rach-id approved these changes Mar 31, 2025

View reviewed changes

rach-id merged commit bbd3b9b into v0.38.x-celestia Mar 31, 2025
24 of 26 checks passed

rach-id deleted the marko/core_changes_v3 branch March 31, 2025 17:26

Conversation

tac0turtle commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

evan-forbes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damiannolan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rach-id commented Mar 26, 2025

Uh oh!

evan-forbes commented Mar 26, 2025

Uh oh!

tac0turtle commented Mar 26, 2025

Uh oh!

tac0turtle commented Mar 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tac0turtle Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tac0turtle commented Mar 12, 2025 •

edited

Loading

tac0turtle Mar 28, 2025 •

edited

Loading