diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dbdc21c098..9cfaa5f14d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -115,20 +115,6 @@ jobs: name: unitTest-coverage path: cover.out - # - name: Data race tests - # run: make test-race - - # # TODO: make it work - # - name: Reproducible build test - # run: | - # make geth - # shasum -a256 ./build/bin/geth > bor1.sha256 - # make geth - # shasum -a256 ./build/bin/geth > bor2.sha256 - # if ! cmp -s bor1.sha256 bor2.sha256; then - # echo >&2 "Reproducible build broken"; cat bor1.sha256; cat bor2.sha256; exit 1 - # fi - integration-tests: if: (github.event.action != 'closed' || github.event.pull_request.merged == true) strategy: @@ -180,3 +166,7 @@ jobs: uses: actions/download-artifact@v6.0.0 - name: Upload coverage to Codecov uses: codecov/codecov-action@v3 + with: + token: ${{ secrets.CODECOV_TOKEN }} + fail_ci_if_error: true + verbose: true diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 38e07f8eea..b92f35a0bf 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -33,14 +33,14 @@ jobs: go-version-file: go.mod - name: Initialize CodeQL - uses: github/codeql-action/init@v2 + uses: github/codeql-action/init@v3 with: languages: ${{ matrix.language }} - name: Autobuild - uses: github/codeql-action/autobuild@v2 + uses: github/codeql-action/autobuild@v3 - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v2 + uses: github/codeql-action/analyze@v3 with: category: '/language:${{matrix.language}}' diff --git a/.github/workflows/kurtosis-e2e.yml b/.github/workflows/kurtosis-e2e.yml index 2f90b33c51..b1b1eece8a 100644 --- a/.github/workflows/kurtosis-e2e.yml +++ b/.github/workflows/kurtosis-e2e.yml @@ -17,6 +17,7 @@ concurrency: env: ENCLAVE_NAME: kurtosis-e2e + POLYCLI_VERSION: v0.1.103 jobs: build-bor: @@ -26,13 +27,13 @@ jobs: - name: Checkout bor uses: actions/checkout@v5 - - name: Build docker image + - name: Build bor docker image run: docker build -t bor:local --file Dockerfile . - - name: Save image + - name: Save bor docker image run: docker save bor:local | gzip > bor-image.tar.gz - - name: Upload image + - name: Upload bor docker image uses: actions/upload-artifact@v4 with: name: bor-image @@ -49,13 +50,13 @@ jobs: repository: 0xPolygon/heimdall-v2 ref: develop - - name: Build docker image + - name: Build heimdall-v2 docker image run: docker build -t heimdall-v2:local --file Dockerfile . - - name: Save image + - name: Save heimdall-v2 docker image run: docker save heimdall-v2:local | gzip > heimdall-v2-image.tar.gz - - name: Upload image + - name: Upload heimdall-v2 docker image uses: actions/upload-artifact@v4 with: name: heimdall-v2-image @@ -79,12 +80,13 @@ jobs: uses: actions/checkout@v5 with: repository: 0xPolygon/kurtosis-pos - ref: hv2/tx-eip1559-gas-price-configs + ref: hv2/eip1559-and-rio-at-128 # This step will free disk space and thus remove any docker images previously built - name: Pre kurtosis run - uses: ./.github/actions/kurtosis-pre-run + uses: ./.github/actions/setup with: + main_branch: develop docker_username: ${{ secrets.DOCKERHUB }} docker_token: ${{ secrets.DOCKERHUB_KEY }} @@ -96,29 +98,29 @@ jobs: path: pos-workflows - name: Copy kurtosis config - run: cp ./pos-workflows/.github/configs/kurtosis-e2e.yml ./kurtosis-e2e.yml + run: cp ./pos-workflows/configs/kurtosis-e2e.yml ./kurtosis-e2e.yml # Load images - - name: Download bor image + - name: Download bor docker image uses: actions/download-artifact@v4 with: name: bor-image - - name: Download heimdall-v2 image + - name: Download heimdall-v2 docker image uses: actions/download-artifact@v4 with: name: heimdall-v2-image - - name: Load bor image + - name: Load bor docker image run: | gunzip -c bor-image.tar.gz | docker load - echo "Loaded bor image:" + echo "Loaded bor docker image:" docker images bor:local --format "{{.ID}} {{.CreatedAt}}" - - name: Load heimdall-v2 image + - name: Load heimdall-v2 docker image run: | gunzip -c heimdall-v2-image.tar.gz | docker load - echo "Loaded heimdall-v2 image:" + echo "Loaded heimdall-v2 docker image:" docker images heimdall-v2:local --format "{{.ID}} {{.CreatedAt}}" # Deploy kurtosis enclave @@ -128,9 +130,19 @@ jobs: - name: Inspect enclave run: kurtosis enclave inspect ${{ env.ENCLAVE_NAME }} - # Run e2e tests - - name: Test state syncs - run: kurtosis service exec ${{ env.ENCLAVE_NAME }} test-runner "bats --filter 'bridge MATIC/POL, ERC20, and ERC721 from L1 to L2 and confirm L2 balances increased' tests/pos/bridge.bats" + - name: Install polycli + run: | + tmp_dir=$(mktemp -d) + curl -L https://github.com/0xPolygon/polygon-cli/releases/download/${{ env.POLYCLI_VERSION }}/polycli_${{ env.POLYCLI_VERSION }}_linux_amd64.tar.gz | tar -xz -C "$tmp_dir" + mv "$tmp_dir"/* /usr/local/bin/polycli + rm -rf "$tmp_dir" + sudo chmod +x /usr/local/bin/polycli + polycli version + + - name: Run smoke tests + id: smoke-tests + working-directory: pos-workflows/tests + run: bash kurtosis_smoke_test.sh - name: Install Go uses: actions/setup-go@v6 @@ -138,19 +150,27 @@ jobs: go-version: 'stable' - name: Run RPC tests + id: rpc-tests working-directory: pos-workflows/tests/rpc_tests run: | export RPC_URL=$(kurtosis port print ${{ env.ENCLAVE_NAME }} l2-el-1-bor-heimdall-v2-validator rpc) export PRIV_KEY="0xd40311b5a5ca5eaeb48dfba5403bde4993ece8eccf4190e98e19fcd4754260ea" go run . --priv-key "$PRIV_KEY" --rpc-url "$RPC_URL" --log-req-res true - - name: Run smoke tests (Checkpoints and Milestones) - working-directory: pos-workflows/tests - run: bash ./kurtosis_smoke_test.sh + # Collect diagnostics on failure + - name: Collect network diagnostics and state dump + if: >- + always() && ( + steps.smoke-tests.outcome == 'failure' || + steps.rpc-tests.outcome == 'failure') + uses: ./pos-workflows/.github/actions/network-diagnostics-and-state-dump + with: + enclave_name: ${{ env.ENCLAVE_NAME }} # Clean up - name: Post kurtosis run if: always() - uses: ./.github/actions/kurtosis-post-run + uses: ./.github/actions/cleanup with: + main_branch: develop enclave_name: ${{ env.ENCLAVE_NAME }} diff --git a/.github/workflows/kurtosis-stateless-e2e.yml b/.github/workflows/kurtosis-stateless-e2e.yml index 7d13bc81bd..2e3dde3d7a 100644 --- a/.github/workflows/kurtosis-stateless-e2e.yml +++ b/.github/workflows/kurtosis-stateless-e2e.yml @@ -1,4 +1,4 @@ -name: Stateless Sync E2E Tests +name: Kurtosis Stateless E2E Tests on: push: @@ -12,12 +12,12 @@ on: types: [opened, synchronize] concurrency: - group: stateless-sync-${{ github.event.pull_request.number || github.ref }} + group: kurtosis-stateless-e2e-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true env: ENCLAVE_NAME: kurtosis-stateless-e2e - POLYCLI_VERSION: v0.1.102 + POLYCLI_VERSION: v0.1.103 jobs: build-bor: @@ -27,13 +27,13 @@ jobs: - name: Checkout bor uses: actions/checkout@v5 - - name: Build docker image + - name: Build bor docker image run: docker build -t bor:local --file Dockerfile . - - name: Save image + - name: Save bor docker image run: docker save bor:local | gzip > bor-image.tar.gz - - name: Upload image + - name: Upload bor docker image uses: actions/upload-artifact@v4 with: name: bor-image @@ -50,13 +50,13 @@ jobs: repository: 0xPolygon/heimdall-v2 ref: develop - - name: Build docker image + - name: Build heimdall-v2 docker image run: docker build -t heimdall-v2:local --file Dockerfile . - - name: Save image + - name: Save heimdall-v2 docker image run: docker save heimdall-v2:local | gzip > heimdall-v2-image.tar.gz - - name: Upload image + - name: Upload heimdall-v2 docker image uses: actions/upload-artifact@v4 with: name: heimdall-v2-image @@ -67,7 +67,7 @@ jobs: needs: - build-bor - build-heimdall-v2 - runs-on: ubuntu24.04-16core-64GB-600SSD-bor + runs-on: ubuntu-latest timeout-minutes: 45 # These permissions are required to log in to GCR permissions: @@ -80,15 +80,28 @@ jobs: uses: actions/checkout@v5 with: repository: 0xPolygon/kurtosis-pos - ref: hv2/tx-eip1559-gas-price-configs + ref: hv2/eip1559-and-rio-at-128 # This step will free disk space and thus remove any docker images previously built - name: Pre kurtosis run - uses: ./.github/actions/kurtosis-pre-run + uses: ./.github/actions/setup with: + main_branch: develop docker_username: ${{ secrets.DOCKERHUB }} docker_token: ${{ secrets.DOCKERHUB_KEY }} + - name: Configure Docker API compatibility for Pumba + run: | + # Pumba uses Docker SDK v23.0.3 (API v1.42), but Docker 29+ requires minimum API v1.44 + # Lower the daemon's minimum API version to maintain compatibility + if [ -f /etc/docker/daemon.json ]; then + sudo jq '. + {"min-api-version": "1.42"}' /etc/docker/daemon.json | sudo tee /etc/docker/daemon.json.tmp > /dev/null + sudo mv /etc/docker/daemon.json.tmp /etc/docker/daemon.json + else + echo '{"min-api-version": "1.42"}' | sudo tee /etc/docker/daemon.json > /dev/null + fi + sudo systemctl restart docker + - name: Checkout pos-workflows uses: actions/checkout@v5 with: @@ -97,31 +110,31 @@ jobs: path: pos-workflows - name: Copy kurtosis config - run: cp ./pos-workflows/.github/configs/kurtosis-stateless-e2e.yml ./kurtosis-stateless-e2e.yml + run: cp ./pos-workflows/configs/kurtosis-stateless-e2e.yml ./kurtosis-stateless-e2e.yml # Load images - - name: Download bor image + - name: Download bor docker image uses: actions/download-artifact@v4 with: name: bor-image - - name: Download heimdall-v2 image + - name: Download heimdall-v2 docker image uses: actions/download-artifact@v4 with: name: heimdall-v2-image - - name: Load bor image + - name: Load bor docker image run: | gunzip -c bor-image.tar.gz | docker load - echo "Loaded bor image:" + echo "Loaded bor docker image:" docker images bor:local --format "{{.ID}} {{.CreatedAt}}" - - name: Load heimdall-v2 image + - name: Load heimdall-v2 docker image run: | gunzip -c heimdall-v2-image.tar.gz | docker load - echo "Loaded heimdall-v2 image:" + echo "Loaded heimdall-v2 docker image:" docker images heimdall-v2:local --format "{{.ID}} {{.CreatedAt}}" - + # Deploy kurtosis enclave - name: Kurtosis run run: kurtosis run --args-file=kurtosis-stateless-e2e.yml --enclave ${{ env.ENCLAVE_NAME }} . @@ -139,16 +152,22 @@ jobs: sudo chmod +x /usr/local/bin/polycli polycli version - - name: Run stateless sync tests + - name: Run stateless e2e tests + id: stateless-tests working-directory: pos-workflows/tests/stateless_tests run: bash kurtosis_stateless_test.sh - - name: Test state syncs (post VeBlop HF) - run: kurtosis service exec ${{ env.ENCLAVE_NAME }} test-runner "bats --filter 'bridge MATIC/POL, ERC20, and ERC721 from L1 to L2 and confirm L2 balances increased' tests/pos/bridge.bats" + # Collect diagnostics on failure + - name: Collect network diagnostics and state dump + if: always() && steps.stateless-tests.outcome == 'failure' + uses: ./pos-workflows/.github/actions/network-diagnostics-and-state-dump + with: + enclave_name: ${{ env.ENCLAVE_NAME }} # Clean up - name: Post kurtosis run if: always() - uses: ./.github/actions/kurtosis-post-run + uses: ./.github/actions/cleanup with: + main_branch: develop enclave_name: ${{ env.ENCLAVE_NAME }} diff --git a/.golangci.yml b/.golangci.yml index 7c75529f61..47c6f8f415 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,7 +1,7 @@ # This file configures github.com/golangci/golangci-lint. version: '2' run: - go: 1.25.5 + go: 1.25.7 tests: true linters: default: none diff --git a/consensus/bor/bor.go b/consensus/bor/bor.go index cfb4286d85..1498ba35a7 100644 --- a/consensus/bor/bor.go +++ b/consensus/bor/bor.go @@ -507,10 +507,15 @@ func (c *Bor) verifyCascadingFields(chain consensus.ChainHeaderReader, header *t parent = chain.GetHeader(header.ParentHash, number-1) } - if parent == nil || parent.Number.Uint64() != number-1 || parent.Hash() != header.ParentHash { + if parent == nil || parent.Hash() != header.ParentHash { return consensus.ErrUnknownAncestor } + // Verify block number continuity + if diff := new(big.Int).Sub(header.Number, parent.Number); diff.Cmp(big.NewInt(1)) != 0 { + return consensus.ErrInvalidNumber + } + // Verify that the gasUsed is <= gasLimit if header.GasUsed > header.GasLimit { return fmt.Errorf("invalid gasUsed: have %d, gasLimit %d", header.GasUsed, header.GasLimit) diff --git a/consensus/bor/bor_test.go b/consensus/bor/bor_test.go index eec1e70b00..45bb84c5b0 100644 --- a/consensus/bor/bor_test.go +++ b/consensus/bor/bor_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/ethereum/go-ethereum/consensus" "github.com/ethereum/go-ethereum/crypto" "github.com/holiman/uint256" "github.com/stretchr/testify/require" @@ -869,3 +870,83 @@ func TestVerifySealRejectsOversizedDifficulty(t *testing.T) { diffErr.Actual, uint64(math.MaxUint64)) } } + +func TestVerifyHeaderRejectsInvalidBlockNumber(t *testing.T) { + t.Parallel() + + privKey, err := crypto.GenerateKey() + require.NoError(t, err) + + signerAddr := crypto.PubkeyToAddress(privKey.PublicKey) + + sp := &fakeSpanner{ + vals: []*valset.Validator{ + {Address: signerAddr, VotingPower: 1}, + }, + } + + borCfg := ¶ms.BorConfig{ + Sprint: map[string]uint64{"0": 64}, + Period: map[string]uint64{"0": 2}, + } + + // Use a fixed past timestamp to avoid "block in the future" errors + chain, b := newChainAndBorForTest(t, sp, borCfg, false, common.Address{}, 1600000000) + + parent := chain.HeaderChain().GetHeaderByNumber(0) + require.NotNil(t, parent) + + // Block number that skips ahead (non-contiguous) + header := &types.Header{ + ParentHash: parent.Hash(), + Number: big.NewInt(10), // Should be 1 + Time: parent.Time + 1000, + Difficulty: big.NewInt(2), + Extra: make([]byte, 32+65), + UncleHash: types.EmptyUncleHash, + GasLimit: parent.GasLimit, + BaseFee: parent.BaseFee, + } + + sigHash := SealHash(header, borCfg) + sig, err := crypto.Sign(sigHash.Bytes(), privKey) + require.NoError(t, err) + copy(header.Extra[len(header.Extra)-65:], sig) + + err = b.VerifyHeader(chain.HeaderChain(), header) + if err == nil { + t.Fatal("expected VerifyHeader to reject non-contiguous block number") + } + if !errors.Is(err, consensus.ErrInvalidNumber) { + t.Fatalf("expected ErrInvalidNumber, got %v", err) + } + + // Test overflow case: parent + 1 + 2^64 (would pass with uint64 truncation) + overflow := new(big.Int).Lsh(big.NewInt(1), 64) + overflow.Add(overflow, parent.Number) + overflow.Add(overflow, big.NewInt(1)) + + header2 := &types.Header{ + ParentHash: parent.Hash(), + Number: overflow, + Time: parent.Time + 1000, + Difficulty: big.NewInt(2), + Extra: make([]byte, 32+65), + UncleHash: types.EmptyUncleHash, + GasLimit: parent.GasLimit, + BaseFee: parent.BaseFee, + } + + sigHash2 := SealHash(header2, borCfg) + sig2, err := crypto.Sign(sigHash2.Bytes(), privKey) + require.NoError(t, err) + copy(header2.Extra[len(header2.Extra)-65:], sig2) + + err = b.VerifyHeader(chain.HeaderChain(), header2) + if err == nil { + t.Fatal("expected VerifyHeader to reject overflow block number") + } + if !errors.Is(err, consensus.ErrInvalidNumber) { + t.Fatalf("expected ErrInvalidNumber for overflow, got %v", err) + } +} diff --git a/consensus/bor/heimdallgrpc/state_sync.go b/consensus/bor/heimdallgrpc/state_sync.go index f4b38d3d41..fa4098ddd3 100644 --- a/consensus/bor/heimdallgrpc/state_sync.go +++ b/consensus/bor/heimdallgrpc/state_sync.go @@ -14,17 +14,21 @@ import ( "github.com/0xPolygon/heimdall-v2/x/clerk/types" ) +const ( + stateSyncTotalTimeout = 1 * time.Minute +) + func (h *HeimdallGRPCClient) StateSyncEvents(ctx context.Context, fromID uint64, to int64) ([]*clerk.EventRecordWithTime, error) { log.Info("Fetching state sync events", "fromID", fromID, "to", to) var err error - ctxWithTimeout, cancel := context.WithTimeout(ctx, defaultTimeout) + globalCtx, cancel := context.WithTimeout(ctx, stateSyncTotalTimeout) defer cancel() // Start the timer and set the request type on the context. start := time.Now() - ctx = heimdall.WithRequestType(ctxWithTimeout, heimdall.StateSyncRequest) + ctx = heimdall.WithRequestType(globalCtx, heimdall.StateSyncRequest) // Defer the metrics call. defer func() { @@ -33,36 +37,47 @@ func (h *HeimdallGRPCClient) StateSyncEvents(ctx context.Context, fromID uint64, eventRecords := make([]*clerk.EventRecordWithTime, 0) - pagination := query.PageRequest{ - Limit: stateFetchLimit, - } + for { + pagination := query.PageRequest{ + Limit: stateFetchLimit, + } - req := &types.RecordListWithTimeRequest{ - FromId: fromID, - ToTime: time.Unix(to, 0), - Pagination: pagination, - } + req := &types.RecordListWithTimeRequest{ + FromId: fromID, + ToTime: time.Unix(to, 0), + Pagination: pagination, + } - res, err := h.clerkQueryClient.GetRecordListWithTime(ctx, req) - if err != nil { - return nil, err - } + var res *types.RecordListWithTimeResponse + pageCtx, pageCancel := context.WithTimeout(ctx, defaultTimeout) + res, err = h.clerkQueryClient.GetRecordListWithTime(pageCtx, req) + pageCancel() + if err != nil { + return nil, err + } - events := res.GetEventRecords() - - for _, event := range events { - eventRecord := &clerk.EventRecordWithTime{ - EventRecord: clerk.EventRecord{ - ID: event.Id, - Contract: common.HexToAddress(event.Contract), - Data: event.Data, - TxHash: common.HexToHash(event.TxHash), - LogIndex: event.LogIndex, - ChainID: event.BorChainId, - }, - Time: event.RecordTime, + events := res.GetEventRecords() + + for _, event := range events { + eventRecord := &clerk.EventRecordWithTime{ + EventRecord: clerk.EventRecord{ + ID: event.Id, + Contract: common.HexToAddress(event.Contract), + Data: event.Data, + TxHash: common.HexToHash(event.TxHash), + LogIndex: event.LogIndex, + ChainID: event.BorChainId, + }, + Time: event.RecordTime, + } + eventRecords = append(eventRecords, eventRecord) } - eventRecords = append(eventRecords, eventRecord) + + if len(events) < stateFetchLimit { + break + } + + fromID += uint64(stateFetchLimit) } log.Info("Fetched state sync events", "fromID", fromID, "to", to) diff --git a/consensus/clique/clique.go b/consensus/clique/clique.go index 2cd24cb7ae..ce9cb948c7 100644 --- a/consensus/clique/clique.go +++ b/consensus/clique/clique.go @@ -352,10 +352,15 @@ func (c *Clique) verifyCascadingFields(chain consensus.ChainHeaderReader, header parent = chain.GetHeader(header.ParentHash, number-1) } - if parent == nil || parent.Number.Uint64() != number-1 || parent.Hash() != header.ParentHash { + if parent == nil || parent.Hash() != header.ParentHash { return consensus.ErrUnknownAncestor } + // Verify block number continuity + if diff := new(big.Int).Sub(header.Number, parent.Number); diff.Cmp(big.NewInt(1)) != 0 { + return consensus.ErrInvalidNumber + } + if parent.Time+c.config.Period > header.Time { return errInvalidTimestamp } diff --git a/consensus/clique/clique_test.go b/consensus/clique/clique_test.go index 157a46c130..65d826c1aa 100644 --- a/consensus/clique/clique_test.go +++ b/consensus/clique/clique_test.go @@ -17,10 +17,12 @@ package clique import ( + "errors" "math/big" "testing" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/consensus" "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/types" @@ -128,3 +130,77 @@ func TestSealHash(t *testing.T) { t.Errorf("have %x, want %x", have, want) } } + +func TestVerifyHeaderRejectsInvalidBlockNumber(t *testing.T) { + var ( + db = rawdb.NewMemoryDatabase() + key, _ = crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291") + addr = crypto.PubkeyToAddress(key.PublicKey) + engine = New(params.AllCliqueProtocolChanges.Clique, db) + ) + + genspec := &core.Genesis{ + Config: params.AllCliqueProtocolChanges, + ExtraData: make([]byte, extraVanity+common.AddressLength+extraSeal), + Alloc: map[common.Address]types.Account{ + addr: {Balance: big.NewInt(10000000000000000)}, + }, + BaseFee: big.NewInt(params.InitialBaseFee), + } + copy(genspec.ExtraData[extraVanity:], addr[:]) + + chain, _ := core.NewBlockChain(rawdb.NewMemoryDatabase(), genspec, engine, nil) + defer chain.Stop() + + parent := chain.CurrentBlock() + + // Block number that skips ahead (non-contiguous) + header := &types.Header{ + ParentHash: parent.Hash(), + Number: big.NewInt(10), // Should be 1 + Time: parent.Time + 1, + Difficulty: diffInTurn, + Extra: make([]byte, extraVanity+extraSeal), + UncleHash: types.EmptyUncleHash, + GasLimit: parent.GasLimit, + BaseFee: parent.BaseFee, + } + + sig, _ := crypto.Sign(SealHash(header).Bytes(), key) + copy(header.Extra[len(header.Extra)-extraSeal:], sig) + + err := engine.VerifyHeader(chain, header) + if err == nil { + t.Fatal("expected VerifyHeader to reject non-contiguous block number") + } + if !errors.Is(err, consensus.ErrInvalidNumber) { + t.Fatalf("expected ErrInvalidNumber, got %v", err) + } + + // Test overflow case: parent + 1 + 2^64 (would pass with uint64 truncation) + overflow := new(big.Int).Lsh(big.NewInt(1), 64) + overflow.Add(overflow, parent.Number) + overflow.Add(overflow, big.NewInt(1)) + + header2 := &types.Header{ + ParentHash: parent.Hash(), + Number: overflow, + Time: parent.Time + 1, + Difficulty: diffInTurn, + Extra: make([]byte, extraVanity+extraSeal), + UncleHash: types.EmptyUncleHash, + GasLimit: parent.GasLimit, + BaseFee: parent.BaseFee, + } + + sig2, _ := crypto.Sign(SealHash(header2).Bytes(), key) + copy(header2.Extra[len(header2.Extra)-extraSeal:], sig2) + + err = engine.VerifyHeader(chain, header2) + if err == nil { + t.Fatal("expected VerifyHeader to reject overflow block number") + } + if !errors.Is(err, consensus.ErrInvalidNumber) { + t.Fatalf("expected ErrInvalidNumber for overflow, got %v", err) + } +} diff --git a/consensus/misc/eip1559/eip1559.go b/consensus/misc/eip1559/eip1559.go index f94b0a2ad2..577e749fb5 100644 --- a/consensus/misc/eip1559/eip1559.go +++ b/consensus/misc/eip1559/eip1559.go @@ -43,6 +43,10 @@ func VerifyEIP1559Header(config *params.ChainConfig, parent, header *types.Heade if header.BaseFee == nil { return errors.New("header is missing baseFee") } + // Verify the parent header is not malformed + if config.IsLondon(parent.Number) && parent.BaseFee == nil { + return errors.New("parent header is missing baseFee") + } // Verify the baseFee is correct based on the parent header. expectedBaseFee := CalcBaseFee(config, parent) if header.BaseFee.Cmp(expectedBaseFee) != 0 { @@ -56,7 +60,7 @@ func VerifyEIP1559Header(config *params.ChainConfig, parent, header *types.Heade // CalcBaseFee calculates the basefee of the header. func CalcBaseFee(config *params.ChainConfig, parent *types.Header) *big.Int { // If the current block is the first EIP-1559 block, return the InitialBaseFee. - if !config.IsLondon(parent.Number) { + if !config.IsLondon(parent.Number) || parent.BaseFee == nil { return new(big.Int).SetUint64(params.InitialBaseFee) } diff --git a/consensus/misc/eip1559/eip1559_test.go b/consensus/misc/eip1559/eip1559_test.go index ca9fe8e4e2..8b7bd8fcb4 100644 --- a/consensus/misc/eip1559/eip1559_test.go +++ b/consensus/misc/eip1559/eip1559_test.go @@ -174,6 +174,177 @@ func TestCalcBaseFeeDelhi(t *testing.T) { } } +// TestCalcBaseFeeNilParent tests that CalcBaseFee doesn't panic when +// the parent's BaseFee is nil. +func TestCalcBaseFeeNilParent(t *testing.T) { + t.Parallel() + + testConfig := config() + + t.Run("nil baseFee for post-London parent returns InitialBaseFee", func(t *testing.T) { + // Create a post-London parent header with nil BaseFee + parent := &types.Header{ + Number: big.NewInt(6), // Post-London because LondonBlock is 5 in test config + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: nil, + } + + // CalcBaseFee should not panic but return InitialBaseFee + result := CalcBaseFee(testConfig, parent) + expected := big.NewInt(params.InitialBaseFee) + + require.NotNil(t, result, "CalcBaseFee should not return nil") + require.Equal(t, expected, result, + "CalcBaseFee should return InitialBaseFee when the parent's BaseFee is nil for post-London block") + }) + + t.Run("pre-London parent with nil baseFee returns InitialBaseFee", func(t *testing.T) { + // Pre-London blocks should have nil BaseFee anyway + parent := &types.Header{ + Number: big.NewInt(4), // Pre-London because LondonBlock is 5 in test config + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: nil, + } + + result := CalcBaseFee(testConfig, parent) + expected := big.NewInt(params.InitialBaseFee) + + require.NotNil(t, result, "CalcBaseFee should not return nil") + require.Equal(t, expected, result, + "CalcBaseFee should return InitialBaseFee for first EIP-1559 block") + }) +} + +// TestVerifyEIP1559HeaderNilParentBaseFee tests that VerifyEIP1559Header rejects post-London parents with nil BaseFee. +func TestVerifyEIP1559HeaderNilParentBaseFee(t *testing.T) { + t.Parallel() + + testConfig := config() + + t.Run("post-London parent with nil BaseFee is rejected", func(t *testing.T) { + // Malicious parent: post-London block with nil BaseFee + parent := &types.Header{ + Number: big.NewInt(6), // Post-London (LondonBlock is 5) + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: nil, + } + + // Child header with valid BaseFee + child := &types.Header{ + Number: big.NewInt(7), + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: big.NewInt(params.InitialBaseFee), + } + + // VerifyEIP1559Header must reject due to nil parent's BaseFee + err := VerifyEIP1559Header(testConfig, parent, child) + require.Error(t, err, "VerifyEIP1559Header must reject nil parent's BaseFee") + require.Contains(t, err.Error(), "parent header is missing baseFee", + "Error message should indicate parent BaseFee is missing") + }) + + t.Run("pre-London parent with nil BaseFee is accepted", func(t *testing.T) { + parent := &types.Header{ + Number: big.NewInt(4), // Pre-London (LondonBlock is 5) + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: nil, // Expected for pre-London blocks + } + + child := &types.Header{ + Number: big.NewInt(5), // LondonBlock + GasLimit: 40000000, // parent.GasLimit * elasticityMultiplier = 20M * 2 + GasUsed: 20000000, + BaseFee: big.NewInt(params.InitialBaseFee), + } + + err := VerifyEIP1559Header(testConfig, parent, child) + require.NoError(t, err, "First London block with InitialBaseFee should be accepted") + }) + + t.Run("post-London parent with valid BaseFee is accepted", func(t *testing.T) { + // Valid parent + parent := &types.Header{ + Number: big.NewInt(6), // Post-London (LondonBlock is 5) + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: big.NewInt(params.InitialBaseFee), + } + + // Valid child + expectedBaseFee := CalcBaseFee(testConfig, parent) + child := &types.Header{ + Number: big.NewInt(7), + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: expectedBaseFee, + } + + err := VerifyEIP1559Header(testConfig, parent, child) + require.NoError(t, err, "Valid parent and child should be accepted") + }) +} + +// TestBatchVerification tests that if a peer sends header batch [A, B] where A has nil BaseFee and future +// timestamp, and B is a child of A, it should not panic but return an error. +func TestBatchVerification(t *testing.T) { + t.Parallel() + + testConfig := config() + + t.Run("batch A->B does not panic", func(t *testing.T) { + // Header A: post-London, nil BaseFee, forwarded to child verification in batch + headerA := &types.Header{ + Number: big.NewInt(6), // Post-London (LondonBlock is 5) + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: nil, + } + + // Header B: child of A, trying to exploit the nil BaseFee + headerB := &types.Header{ + Number: big.NewInt(7), + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: big.NewInt(params.InitialBaseFee), + } + + // verify B with A as parent doesn't panic but returns the expected error + var err error + require.NotPanics(t, func() { + err = VerifyEIP1559Header(testConfig, headerA, headerB) + }, "VerifyEIP1559Header must not panic when parent.BaseFee is nil") + + require.Error(t, err, "VerifyEIP1559Header must reject child when parent.BaseFee is nil") + require.Contains(t, err.Error(), "parent header is missing baseFee", + "Error must indicate parent BaseFee issue") + }) + + t.Run("CalcBaseFee called directly does not panic", func(t *testing.T) { + // Header with nil BaseFee + header := &types.Header{ + Number: big.NewInt(6), // Post-London (LondonBlock is 5) + GasLimit: 20000000, + GasUsed: 10000000, + BaseFee: nil, + } + + // CalcBaseFee doesn't panic + var result *big.Int + require.NotPanics(t, func() { + result = CalcBaseFee(testConfig, header) + }, "CalcBaseFee must not panic when parent.BaseFee is nil") + + require.NotNil(t, result, "CalcBaseFee should return non-nil result") + require.Equal(t, big.NewInt(params.InitialBaseFee), result, + "CalcBaseFee should return InitialBaseFee as fallback") + }) +} + func TestCalcParentGasTarget(t *testing.T) { t.Parallel() @@ -271,6 +442,9 @@ func TestCalcBaseFeeDandeli(t *testing.T) { t.Parallel() testConfig := copyConfig(config()) + // Create a new Bor config to avoid modifying the shared one + borCopy := *testConfig.Bor + testConfig.Bor = &borCopy testConfig.Bor.BhilaiBlock = big.NewInt(8) testConfig.Bor.DandeliBlock = big.NewInt(20) diff --git a/crypto/crypto.go b/crypto/crypto.go index 4c32b19647..9fa4320d3a 100644 --- a/crypto/crypto.go +++ b/crypto/crypto.go @@ -203,6 +203,10 @@ func UnmarshalPubkey(pub []byte) (*ecdsa.PublicKey, error) { if x == nil { return nil, errInvalidPubkey } + // Check coordinates are < P, to protect against potential misses in Unmarshal implementations. + if x.Cmp(S256().Params().P) >= 0 || y.Cmp(S256().Params().P) >= 0 { + return nil, errInvalidPubkey + } if !S256().IsOnCurve(x, y) { return nil, errInvalidPubkey } diff --git a/crypto/secp256k1/curve.go b/crypto/secp256k1/curve.go index 705d387ddf..b4de0f3763 100644 --- a/crypto/secp256k1/curve.go +++ b/crypto/secp256k1/curve.go @@ -92,6 +92,11 @@ func (bitCurve *BitCurve) Params() *elliptic.CurveParams { // IsOnCurve returns true if the given (x,y) lies on the BitCurve. func (bitCurve *BitCurve) IsOnCurve(x, y *big.Int) bool { + // Reject non-canonical encodings to ensure coordinates are < P + if x.Cmp(bitCurve.P) >= 0 || y.Cmp(bitCurve.P) >= 0 { + return false + } + // y² = x³ + b y2 := new(big.Int).Mul(y, y) //y² y2.Mod(y2, bitCurve.P) //y²%P @@ -287,6 +292,11 @@ func (bitCurve *BitCurve) Unmarshal(data []byte) (x, y *big.Int) { x = new(big.Int).SetBytes(data[1 : 1+byteLen]) y = new(big.Int).SetBytes(data[1+byteLen:]) + // Reject non-canonical encodings to ensure coordinates are < P + if x.Cmp(bitCurve.P) >= 0 || y.Cmp(bitCurve.P) >= 0 { + return nil, nil + } + return } diff --git a/crypto/secp256k1/ext.h b/crypto/secp256k1/ext.h index 1c485e2603..470feb1ec2 100644 --- a/crypto/secp256k1/ext.h +++ b/crypto/secp256k1/ext.h @@ -109,8 +109,13 @@ int secp256k1_ext_scalar_mul(const secp256k1_context* ctx, unsigned char *point, ARG_CHECK(scalar != NULL); (void)ctx; - secp256k1_fe_set_b32_limit(&feX, point); - secp256k1_fe_set_b32_limit(&feY, point+32); + // Reject non-canonical field elements by ensuring coordinates are < P + if (!secp256k1_fe_set_b32_limit(&feX, point)) { + return 0; + } + if (!secp256k1_fe_set_b32_limit(&feY, point+32)) { + return 0; + } secp256k1_ge_set_xy(&ge, &feX, &feY); secp256k1_scalar_set_b32(&s, scalar, &overflow); if (overflow || secp256k1_scalar_is_zero(&s)) { diff --git a/crypto/signature_nocgo.go b/crypto/signature_nocgo.go index d76127c258..ae6352e356 100644 --- a/crypto/signature_nocgo.go +++ b/crypto/signature_nocgo.go @@ -164,6 +164,13 @@ type btCurve struct { *secp256k1.KoblitzCurve } +func (curve btCurve) IsOnCurve(x, y *big.Int) bool { + if x.Cmp(secp256k1.Params().P) >= 0 || y.Cmp(secp256k1.Params().P) >= 0 { + return false + } + return curve.KoblitzCurve.IsOnCurve(x, y) +} + // Marshal converts a point given as (x, y) into a byte slice. func (curve btCurve) Marshal(x, y *big.Int) []byte { byteLen := (curve.Params().BitSize + 7) / 8 @@ -189,5 +196,11 @@ func (curve btCurve) Unmarshal(data []byte) (x, y *big.Int) { } x = new(big.Int).SetBytes(data[1 : 1+byteLen]) y = new(big.Int).SetBytes(data[1+byteLen:]) + + // Reject non-canonical encodings to ensure coordinates are < P + if x.Cmp(curve.Params().P) >= 0 || y.Cmp(curve.Params().P) >= 0 { + return nil, nil + } + return } diff --git a/crypto/signature_test.go b/crypto/signature_test.go index 437669f114..4d9f94e0f4 100644 --- a/crypto/signature_test.go +++ b/crypto/signature_test.go @@ -19,6 +19,7 @@ package crypto import ( "bytes" "crypto/ecdsa" + "math/big" "reflect" "testing" @@ -174,3 +175,116 @@ func BenchmarkDecompressPubkey(b *testing.B) { } } } + +// TestNonCanonicalCoordinates verifies that public keys with coordinates >= P +// are rejected during unmarshalling +func TestNonCanonicalCoordinates(t *testing.T) { + P := S256().Params().P + + // Test case 1: X = P + t.Run("X_equals_P", func(t *testing.T) { + pubkey := make([]byte, 65) + pubkey[0] = 4 // uncompressed + P.FillBytes(pubkey[1:33]) + // Y = 1 + pubkey[64] = 1 + + // Should be rejected by UnmarshalPubkey + _, err := UnmarshalPubkey(pubkey) + if err == nil { + t.Fatal("expected error for X >= P, got nil") + } + }) + + // Test case 2: X = P + 1 + t.Run("X_greater_than_P", func(t *testing.T) { + xPlus1 := new(big.Int).Add(P, big.NewInt(1)) + pubkey := make([]byte, 65) + pubkey[0] = 4 + xPlus1.FillBytes(pubkey[1:33]) + pubkey[64] = 1 // Y = 1 + + _, err := UnmarshalPubkey(pubkey) + if err == nil { + t.Fatal("expected error for X > P, got nil") + } + }) + + // Test case 3: Y = P + t.Run("Y_equals_P", func(t *testing.T) { + pubkey := make([]byte, 65) + pubkey[0] = 4 + pubkey[32] = 1 // X = 1 + P.FillBytes(pubkey[33:65]) + + _, err := UnmarshalPubkey(pubkey) + if err == nil { + t.Fatal("expected error for Y >= P, got nil") + } + }) + + // Test case 4: Valid coordinates + t.Run("valid_coordinates", func(t *testing.T) { + params := S256().Params() + pubkey := make([]byte, 65) + pubkey[0] = 4 + params.Gx.FillBytes(pubkey[1:33]) + params.Gy.FillBytes(pubkey[33:65]) + + pub, err := UnmarshalPubkey(pubkey) + if err != nil { + t.Fatalf("valid coordinates rejected: %v", err) + } + if pub.X.Cmp(params.Gx) != 0 || pub.Y.Cmp(params.Gy) != 0 { + t.Fatal("unmarshalled coordinates don't match") + } + }) + + // Test case 5: Verify CompressPubkey doesn't panic on valid keys + t.Run("compress_valid_key", func(t *testing.T) { + key, err := GenerateKey() + if err != nil { + t.Fatalf("key generation failed: %v", err) + } + + compressed := CompressPubkey(&key.PublicKey) + if len(compressed) != 33 { + t.Fatalf("wrong compressed length: %d", len(compressed)) + } + }) +} + +// TestIsOnCurveNonCanonical verifies that IsOnCurve rejects non-canonical +// coordinates, preventing misuse by callers who might bypass Unmarshal +func TestIsOnCurveNonCanonical(t *testing.T) { + curve := S256() + P := curve.Params().P + + // Test case 1: X = P should be rejected + t.Run("X_equals_P", func(t *testing.T) { + x := new(big.Int).Set(P) + y := big.NewInt(1) + + if curve.IsOnCurve(x, y) { + t.Fatal("IsOnCurve accepted X = P (non-canonical)") + } + }) + + // Test case 2: Y = P should be rejected + t.Run("Y_equals_P", func(t *testing.T) { + x := big.NewInt(1) + y := new(big.Int).Set(P) + + if curve.IsOnCurve(x, y) { + t.Fatal("IsOnCurve accepted Y = P (non-canonical)") + } + }) + + // Test case 3: Valid generator point should still work + t.Run("valid_generator", func(t *testing.T) { + params := curve.Params() + if !curve.IsOnCurve(params.Gx, params.Gy) { + t.Fatal("IsOnCurve rejected valid generator point") + } + }) +} diff --git a/go.mod b/go.mod index d630adbc8b..3c7902a039 100644 --- a/go.mod +++ b/go.mod @@ -1,7 +1,7 @@ module github.com/ethereum/go-ethereum // Note: Change the go image version in Dockerfile if you change this. -go 1.25.5 +go 1.25.7 require ( github.com/0xPolygon/crand v1.0.3 diff --git a/p2p/discover/v4wire/v4wire.go b/p2p/discover/v4wire/v4wire.go index 81aafbe8b3..3619f10236 100644 --- a/p2p/discover/v4wire/v4wire.go +++ b/p2p/discover/v4wire/v4wire.go @@ -306,6 +306,11 @@ func DecodePubkey(curve elliptic.Curve, e Pubkey) (*ecdsa.PublicKey, error) { p.X.SetBytes(e[:half]) p.Y.SetBytes(e[half:]) + // Reject non-canonical coordinates (X >= P or Y >= P) + if p.X.Cmp(p.Curve.Params().P) >= 0 || p.Y.Cmp(p.Curve.Params().P) >= 0 { + return nil, ErrBadPoint + } + if !p.Curve.IsOnCurve(p.X, p.Y) { return nil, ErrBadPoint } diff --git a/p2p/discover/v4wire/v4wire_test.go b/p2p/discover/v4wire/v4wire_test.go index dbc391b433..756c266ce5 100644 --- a/p2p/discover/v4wire/v4wire_test.go +++ b/p2p/discover/v4wire/v4wire_test.go @@ -18,6 +18,7 @@ package v4wire import ( "encoding/hex" + "math/big" "net" "reflect" "testing" @@ -136,3 +137,127 @@ func hexPubkey(h string) (ret Pubkey) { return ret } + +// TestDecodePubkeyNonCanonical verifies that DecodePubkey rejects non-canonical +// secp256k1 coordinates (X >= P or Y >= P) +func TestDecodePubkeyNonCanonical(t *testing.T) { + curve := crypto.S256() + P := curve.Params().P + + // Test case 1: X = P + t.Run("X_equals_P", func(t *testing.T) { + var pubkey Pubkey + // First half: X = P + P.FillBytes(pubkey[:32]) + // Second half: Y = 1 + pubkey[63] = 1 + + _, err := DecodePubkey(curve, pubkey) + if err == nil { + t.Fatal("expected error for X >= P, got nil") + } + if err != ErrBadPoint { + t.Fatalf("expected ErrBadPoint, got %v", err) + } + }) + + // Test case 2: X = P + 1 + t.Run("X_greater_than_P", func(t *testing.T) { + var pubkey Pubkey + xPlus1 := new(big.Int).Add(P, big.NewInt(1)) + xPlus1.FillBytes(pubkey[:32]) + pubkey[63] = 1 // Y = 1 + + _, err := DecodePubkey(curve, pubkey) + if err == nil { + t.Fatal("expected error for X > P, got nil") + } + if err != ErrBadPoint { + t.Fatalf("expected ErrBadPoint, got %v", err) + } + }) + + // Test case 3: Y = P + t.Run("Y_equals_P", func(t *testing.T) { + var pubkey Pubkey + pubkey[31] = 1 // X = 1 + P.FillBytes(pubkey[32:]) + + _, err := DecodePubkey(curve, pubkey) + if err == nil { + t.Fatal("expected error for Y >= P, got nil") + } + if err != ErrBadPoint { + t.Fatalf("expected ErrBadPoint, got %v", err) + } + }) + + // Test case 4: Valid coordinates + t.Run("valid_coordinates", func(t *testing.T) { + params := curve.Params() + var pubkey Pubkey + params.Gx.FillBytes(pubkey[:32]) + params.Gy.FillBytes(pubkey[32:]) + + pub, err := DecodePubkey(curve, pubkey) + if err != nil { + t.Fatalf("valid coordinates rejected: %v", err) + } + if pub.X.Cmp(params.Gx) != 0 || pub.Y.Cmp(params.Gy) != 0 { + t.Fatal("decoded coordinates don't match") + } + }) + + // Test case 5: Valid random key round-trip + t.Run("valid_random_key", func(t *testing.T) { + key, err := crypto.GenerateKey() + if err != nil { + t.Fatalf("key generation failed: %v", err) + } + + encoded := EncodePubkey(&key.PublicKey) + decoded, err := DecodePubkey(curve, encoded) + if err != nil { + t.Fatalf("decoding valid key failed: %v", err) + } + + if decoded.X.Cmp(key.X) != 0 || decoded.Y.Cmp(key.Y) != 0 { + t.Fatal("round-trip mismatch") + } + }) +} + +// TestNeighborsNonCanonicalNode verifies that a Neighbors packet with a node +// containing non-canonical coordinates is properly rejected +func TestNeighborsNonCanonicalNode(t *testing.T) { + curve := crypto.S256() + P := curve.Params().P + + // Create a Neighbors packet with X = P + var maliciousID Pubkey + P.FillBytes(maliciousID[:32]) + maliciousID[63] = 1 // Y = 1 + + neighbors := &Neighbors{ + Nodes: []Node{ + { + ID: maliciousID, + IP: net.ParseIP("127.0.0.1").To4(), + UDP: 30303, + TCP: 30303, + }, + }, + Expiration: 0, + } + + // Try to decode the node ID + _, err := DecodePubkey(curve, neighbors.Nodes[0].ID) + if err == nil { + t.Fatal("expected error for node with X >= P, got nil") + } + if err != ErrBadPoint { + t.Fatalf("expected ErrBadPoint, got %v", err) + } + + t.Log("Malicious node ID correctly rejected") +} diff --git a/params/version.go b/params/version.go index 3e522e1d0a..7b59e13044 100644 --- a/params/version.go +++ b/params/version.go @@ -25,7 +25,7 @@ import ( const ( VersionMajor = 2 // Major version component of the current release VersionMinor = 5 // Minor version component of the current release - VersionPatch = 8 // Patch version component of the current release + VersionPatch = 9 // Patch version component of the current release VersionMeta = "" // Version metadata to append to the version string )