Spec update #1858

kirugan · 2024-05-06T09:43:11Z

codecov · 2024-05-22T10:54:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.40%. Comparing base (4e6dcd7) to head (8670ea7).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1858      +/-   ##
==========================================
- Coverage   75.43%   75.40%   -0.03%     
==========================================
  Files          97       97              
  Lines        8342     8340       -2     
==========================================
- Hits         6293     6289       -4     
- Misses       1519     1520       +1     
- Partials      530      531       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

weiihann · 2024-06-14T16:01:35Z

p2p/sync.go

-
-		s.log.Infow("Start Pipeline", "Random node height", randHeight, "Current height", nextHeight-1, "Start", nextHeight, "End",
-			nextHeight+min(blockBehind, maxBlocks))
+		s.log.Infow("Start Pipeline", "Random node height", randHeight, "Current height", nextHeight-1, "Start", nextHeight)


randHeight will no longer be used, so it will always be 0. Do we need to remove it?

Would like to ask, if we no longer choose a random peer and sync with it, how does the current implementation choose its peers?

I would also recommend adding some sort of check such that if no peers are available, then don't bother trying to sync at all, perhaps retry every certain interval. This is because I've been getting a lot of these messages while attempting to sync when there are no peers:

23:53:22.370 14/06/2024 +08:00 ERROR p2p/sync.go:79 Failed to get block headers parts{"err": "no peers available"} github.com/NethermindEth/juno/p2p.(*syncService).start /Users/han/Documents/Codes/juno/p2p/sync.go:79 github.com/NethermindEth/juno/p2p.(*Service).Run /Users/han/Documents/Codes/juno/p2p/p2p.go:212 github.com/NethermindEth/juno/node.(*Node).Run.func3 /Users/han/Documents/Codes/juno/node/node.go:353 github.com/sourcegraph/conc/panics.(*Catcher).Try /Users/han/go/1.22.4/pkg/mod/github.com/sourcegraph/conc@v0.3.0/panics/panics.go:23 github.com/sourcegraph/conc.(*WaitGroup).Go.func1 /Users/han/go/1.22.4/pkg/mod/github.com/sourcegraph/conc@v0.3.0/waitgroup.go:32

IronGauntlets

There is some todo I think we can work but we can complete them in a separate PR. Also, there is some unused code which should be removed (such as randHeight and onBlockBodiesRequest.

In general, the PR looks good, I have left some questions and suggestions.

Here are some of my thoughts on the sync process:

Good first step towards using the new conventions.
To fully utilise the Stream convention we should request more than 1 block parts, however, this would complicate the sync process a lot.
We should try to benchmark the current sync process so that it can be compared to any new ones in the future. It may be the case that the simple one is more efficient.

IronGauntlets · 2024-06-13T20:05:54Z

adapters/core2p2p/block.go

-		GasPrice:        AdaptFelt(header.GasPrice),
+		GasPriceFri:     AdaptUint128(header.GasPrice),
+		Signatures:      utils.Map(header.Signatures, AdaptSignature),
+		// todo(kirill) set these fields


I am assuming these fields will have to be set after 0.13.2 update, right?

IronGauntlets · 2024-06-13T20:07:39Z

adapters/core2p2p/block.go

+	default:
+		panic(fmt.Errorf("unknown L1DAMode %v", da))


nit: given the data is coming from our db I think we can assume it will be correct.

yep, I can change it to panic("unreachable") but without it won't work because switch must have default case that either panic or return smth.

IronGauntlets · 2024-06-13T20:28:06Z

adapters/core2p2p/felt.go

+func AdaptUint128(f *felt.Felt) *spec.Uint128 {
+	// bits represents value in little endian byte order
+	// i.e. first is least significant byte
+	bits := f.Bits()
+	// todo what should we do with the rest of bytes?
+	return &spec.Uint128{
+		Low:  bits[0],
+		High: bits[1],
+	}
+}


Do you think they are not using felt to reduce the size of the message?.

Having 128 bits to represent a number seems a bit weird, not sure why uint64 is not enough.

// todo what should we do with the rest of bytes?
I don't think the rest of the words would be used as this is only used for Gas/DataGas prices

IronGauntlets · 2024-06-13T22:43:09Z

adapters/core2p2p/transaction.go

 func adaptDeployTransaction(tx *core.DeployTransaction) *spec.Transaction_Deploy_ {
 	return &spec.Transaction_Deploy_{
 		Deploy: &spec.Transaction_Deploy{
 			ClassHash:   AdaptHash(tx.ClassHash),
 			AddressSalt: AdaptFelt(tx.ContractAddressSalt),
 			Calldata:    AdaptFeltSlice(tx.ConstructorCallData),
+			Version:     0, // todo(kirill) remove field from spec? tx is deprecated so no future versions


I think that is a good idea

IronGauntlets · 2024-06-13T22:45:36Z

adapters/p2p2core/class.go

+		adaptEP := func(points []*spec.EntryPoint) []core.EntryPoint {
+			// usage of NonNilSlice is essential because relevant core class fields are non nil


So can EntryPoints be empty or do you think there is a bug in Pathfinder?

IronGauntlets · 2024-06-16T22:58:34Z

p2p/starknet/client.go

+	err = stream.SetReadDeadline(time.Now().Add(readTimeout))
+	if err != nil {
+		return nil, err
+	}
+


Which issue caused you to introduce a timeout?

IronGauntlets · 2024-06-16T23:32:01Z

p2p/sync.go

+			if !coreBlock.Hash.Equal(h) {
+				spew.Dump("Receipts number:", coreBlock.Receipts, events)
+				spew.Dump("Hash mismtach", coreBlock)
+				os.Exit(1)
+			}


consider removing this debug code?

IronGauntlets · 2024-06-16T23:47:22Z

p2p/starknet/handlers.go

+	fin := newFin(&spec.BlockBodiesResponse{
+		BodyMessage: &spec.BlockBodiesResponse_Fin{},
+	})
+


I don't see why we need to keep this function. let's delete it.

IronGauntlets · 2024-06-17T00:01:12Z

p2p/starknet/handlers.go

 	log      utils.SimpleLogger
+
+	ctx    context.Context


I have always read that storing a context is not a good idea.

I can see that it is used in different protocol handlers protocol handlers when creating a stream. However, we could pass context from the setProtocolHanderls() and avoid this storage. The cancel function can be set when HeadersHandler() or any other handler function is called.

(I would be happy if this change could be made in a separate PR)

IronGauntlets · 2024-06-17T00:08:41Z

p2p/starknet/handlers.go

+			for _, msg := range messages {
+				// push generated msg to caller
+				if !yield(msg) {
+					// if caller is not interested in remaining data (example: connection to a peer is closed) exit
+					// note that in this case we won't send finMsg
+					return
+				}
+			}


If I am not mistaken, this is the only part which is different from processIterationRequest. Do you think we could pass this as a function and reduce the duplication?

kirugan · 2024-06-17T08:11:29Z

There is some todo I think we can work but we can complete them in a separate PR. Also, there is some unused code which should be removed (such as randHeight and onBlockBodiesRequest.

In general, the PR looks good, I have left some questions and suggestions.

Here are some of my thoughts on the sync process:
* Good first step towards using the new conventions.

* To fully utilise the Stream convention we should request more than 1 block parts, however, this would complicate the sync process a lot.

* We should try to benchmark the current sync process so that it can be compared to any new ones in the future. It may be the case that the simple one is more efficient.

Totally agree

kirugan added 5 commits May 2, 2024 15:40

Refactor p2p handlers: move commong iterator logic to a helper

e4c8d1e

Update p2p spec

c5c0f70

Fix some compilation errors

31a573b

Merge remote-tracking branch 'origin/main' into kirugan/p2p-spec-update

0b57c63

New methods, updated structs

c75e18d

kirugan added 3 commits May 22, 2024 18:37

Debug commit

ce575cb

Restructure adapter, update debugging output

e6ba754

Working with final spec

77bb306

wojciechos temporarily deployed to Development May 23, 2024 15:35 — with GitHub Actions Inactive

wojciechos had a problem deploying to Staging May 23, 2024 15:36 — with GitHub Actions Failure

kirugan added 7 commits May 29, 2024 14:01

Recent changes

fac9668

Merge remote-tracking branch 'origin/main' into kirugan/p2p-spec-update

c928445

Added debug code

0ba0ed0

Remove unused code

f3a1f3e

Add spec.StateDiffsResponse_DeclaredClass message

a856d29

Remove debugging print & unused code, fix replaced classes.

0d4e20f

Latest fixes

1ddbb84

wojciechos temporarily deployed to Development June 5, 2024 10:52 — with GitHub Actions Inactive

wojciechos had a problem deploying to Staging June 5, 2024 10:54 — with GitHub Actions Failure

kirugan added 2 commits June 5, 2024 15:38

Fix linter issues

7fb2340

Cleanup code

dff460e

kirugan marked this pull request as ready for review June 5, 2024 11:55

kirugan added 2 commits June 5, 2024 17:31

Fix nil entry points in adapters

f8e518d

Fix stateReader for 0 block

203bc17

kirugan requested review from weiihann and IronGauntlets June 14, 2024 09:37

Merge remote-tracking branch 'origin/main' into kirugan/p2p-spec-update

8670ea7

weiihann reviewed Jun 14, 2024

View reviewed changes

IronGauntlets approved these changes Jun 17, 2024

View reviewed changes

wojciechos had a problem deploying to Development June 17, 2024 07:51 — with GitHub Actions Failure

weiihann approved these changes Jun 19, 2024

View reviewed changes

kirugan force-pushed the main branch from dfbedca to ad21b2e Compare June 28, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spec update #1858

Spec update #1858

kirugan commented May 6, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading

weiihann Jun 14, 2024

IronGauntlets left a comment

IronGauntlets Jun 13, 2024

IronGauntlets Jun 13, 2024

kirugan Jun 17, 2024

IronGauntlets Jun 13, 2024

IronGauntlets Jun 13, 2024

IronGauntlets Jun 13, 2024

IronGauntlets Jun 16, 2024

IronGauntlets Jun 16, 2024

IronGauntlets Jun 16, 2024

IronGauntlets Jun 17, 2024

IronGauntlets Jun 17, 2024

kirugan commented Jun 17, 2024

		adaptEP := func(points []*spec.EntryPoint) []core.EntryPoint {
		// usage of NonNilSlice is essential because relevant core class fields are non nil

Spec update #1858

Are you sure you want to change the base?

Spec update #1858

Conversation

kirugan commented May 6, 2024 • edited Loading

codecov bot commented May 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

IronGauntlets left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirugan commented Jun 17, 2024

kirugan commented May 6, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading