`commitBlobTransaction`: fix nil pointer #23

blockchaindevsh · 2024-12-26T08:40:05Z

Fix this panic found by @iteyelmp :

ERROR[12-26|08:19:43.819] RPC method engine_forkchoiceUpdatedV3 crashed: runtime error: invalid memory address or nil pointer dereference
goroutine 4461 [running]:
github.com/ethereum/go-ethereum/rpc.(*callback).call.func1()
        github.com/ethereum/go-ethereum/rpc/service.go:199 +0x85
panic({0x18a3440?, 0x351a4f0?})
        runtime/panic.go:770 +0x132
github.com/ethereum/go-ethereum/miner.(*Miner).commitBlobTransaction(0xc00184c330?, 0xc0017fe930?, 0x786025?)
        github.com/ethereum/go-ethereum/miner/worker.go:399 +0x4d
github.com/ethereum/go-ethereum/miner.(*Miner).commitTransaction(0xc000504280, 0xc0006ea6c0, 0xc00183c120)
        github.com/ethereum/go-ethereum/miner/worker.go:364 +0x30d
github.com/ethereum/go-ethereum/miner.(*Miner).generateWork(0xc000504280, 0xc0017febf0, 0x5e?)
        github.com/ethereum/go-ethereum/miner/worker.go:157 +0x2a9
github.com/ethereum/go-ethereum/miner.(*Miner).buildPayload(0xc000504280, 0xc0017ff1e0, 0xfa?)
        github.com/ethereum/go-ethereum/miner/payload_building.go:311 +0x153
github.com/ethereum/go-ethereum/miner.(*Miner).BuildPayload(...)
        github.com/ethereum/go-ethereum/miner/miner.go:187
github.com/ethereum/go-ethereum/eth/catalyst.(*ConsensusAPI).forkchoiceUpdated(0xc000cb4960, {{0x58, 0x41, 0xfc, 0x2d, 0x60, 0xf9, 0x99, 0xd2, 0x12, ...}, ...}, ...)
        github.com/ethereum/go-ethereum/eth/catalyst/api.go:483 +0x2e45
github.com/ethereum/go-ethereum/eth/catalyst.(*ConsensusAPI).ForkchoiceUpdatedV3(0xc000cb4960?, {{0x58, 0x41, 0xfc, 0x2d, 0x60, 0xf9, 0x99, 0xd2, 0x12, ...}, ...}, ...)
        github.com/ethereum/go-ethereum/eth/catalyst/api.go:251 +0x3fa
reflect.Value.call({0xc000dc10e0?, 0xc0001bd510?, 0x60?}, {0x1b246a6, 0x4}, {0xc00183c060, 0x3, 0xc00183c060?})
        reflect/value.go:596 +0xca6
reflect.Value.Call({0xc000dc10e0?, 0xc0001bd510?, 0x2?}, {0xc00183c060?, 0x16?, 0x16?})
        reflect/value.go:380 +0xb9
github.com/ethereum/go-ethereum/rpc.(*callback).call(0xc0005db5c0, {0x28b8978, 0xc0006a7180}, {0xc0017ee5a0, 0x1a}, {0xc00164b7d0, 0x2, 0x4e14af?})
        github.com/ethereum/go-ethereum/rpc/service.go:205 +0x36d
github.com/ethereum/go-ethereum/rpc.(*handler).runMethod(0xc00165f500?, {0x28b8978?, 0xc0006a7180?}, 0xc000176700, 0x2?, {0xc00164b7d0?, 0xc001907dc0?, 0x41e6b8?})
        github.com/ethereum/go-ethereum/rpc/handler.go:568 +0x3c
github.com/ethereum/go-ethereum/rpc.(*handler).handleCall(0xc0006ad040, 0xc00164b710, 0xc000176700)
        github.com/ethereum/go-ethereum/rpc/handler.go:515 +0x22f
github.com/ethereum/go-ethereum/rpc.(*handler).handleCallMsg(0xc0006ad040, 0xc00164b710, 0xc000176700)
        github.com/ethereum/go-ethereum/rpc/handler.go:473 +0x22d
github.com/ethereum/go-ethereum/rpc.(*handler).handleNonBatchCall(0xc0006ad040, 0xc00164b710, 0xc000176700)
        github.com/ethereum/go-ethereum/rpc/handler.go:299 +0x18a
github.com/ethereum/go-ethereum/rpc.(*handler).handleMsg.func1.1(0x28b8978?)
        github.com/ethereum/go-ethereum/rpc/handler.go:272 +0x25
github.com/ethereum/go-ethereum/rpc.(*handler).startCallProc.func1()
        github.com/ethereum/go-ethereum/rpc/handler.go:390 +0xbe
created by github.com/ethereum/go-ethereum/rpc.(*handler).startCallProc in goroutine 31
        github.com/ethereum/go-ethereum/rpc/handler.go:386 +0x79

miner/worker.go

syntrust · 2024-12-27T07:47:28Z

miner/worker.go

@@ -396,7 +396,7 @@ func (miner *Miner) commitBlobTransaction(env *environment, tx *types.Transactio
 	// isn't really a better place right now. The blob gas limit is checked at block validation time
 	// and not during execution. This means core.ApplyTransaction will not return an error if the
 	// tx has too many blobs. So we have to explicitly check it here.
-	if (env.blobs+len(sc.Blobs))*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock {


If the issue is due to sc == nil, I prefer to handle sc directly with if sc != nil {}, as the code is self-explanatory and doesn’t need additional comments.

Sorry, I don't think so, more safety checking is better than no checking.

I mean check ("handle" is misleading)sc directly (instead of tx.BlobHashes()).

qzhodl · 2024-12-27T08:54:17Z

miner/worker.go

@@ -396,7 +396,7 @@ func (miner *Miner) commitBlobTransaction(env *environment, tx *types.Transactio
 	// isn't really a better place right now. The blob gas limit is checked at block validation time


@qizhou suggests addressing this case at line 392 to make the code self-explanatory and explicitly handle all possible scenarios.

Suggested change

// isn't really a better place right now. The blob gas limit is checked at block validation time

nblobs := 0

if sc == nil {

if env.isEffectivelySequencing() /* we want to allow blob tx without blobs when it's deriving */ {

panic("blob transaction without blobs in sequencing")

} else { // deriving, which does not have the sidecar

nblobs = len(tx.BlobHashes())

}

} else {

if !env.isEffectivelySequencing() {

panic("blob transaction with blobs in derivation")

}

nblobs = len(sc.Blobs)

}

// should we check tx.BlobHashes() matches that of sc.Blobs.BlobHashes()?

// Checking against blob gas limit: It's kind of ugly to perform this check here, but there

// isn't really a better place right now. The blob gas limit is checked at block validation time

// and not during execution. This means core.ApplyTransaction will not return an error if the

// tx has too many blobs. So we have to explicitly check it here.

if (env.blobs+nblobs)*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock {

return errors.New("max data blobs reached")

}

Done in 80a30e2.

qzhodl · 2024-12-27T08:56:53Z

miner/worker.go

@@ -396,7 +396,7 @@ func (miner *Miner) commitBlobTransaction(env *environment, tx *types.Transactio
 	// isn't really a better place right now. The blob gas limit is checked at block validation time
 	// and not during execution. This means core.ApplyTransaction will not return an error if the
 	// tx has too many blobs. So we have to explicitly check it here.
-	if (env.blobs+len(sc.Blobs))*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock {


Should we check whether len(tx.BlobHashes()) matches len(sc.Blobs), or is this validation handled elsewhere in the code?

When sc != nil, it must be sequencing. In this mode, the blob tx must be pulled from the tx pool, which has already done the validation. So it's not necessary to check it again here.

fix nil pointer

cc702b9

blockchaindevsh requested review from qizhou, qzhodl, dajuguan and syntrust December 26, 2024 08:40

qzhodl reviewed Dec 26, 2024

View reviewed changes

miner/worker.go Outdated Show resolved Hide resolved

qzhodl approved these changes Dec 27, 2024

View reviewed changes

add comment

b2bc505

blockchaindevsh force-pushed the fix_commitBlobTransaction branch from 5919b26 to b2bc505 Compare December 27, 2024 01:06

syntrust reviewed Dec 27, 2024

View reviewed changes

qzhodl reviewed Dec 27, 2024

View reviewed changes

address comments

80a30e2

blockchaindevsh force-pushed the fix_commitBlobTransaction branch from fea317d to 80a30e2 Compare December 27, 2024 09:26

blockchaindevsh mentioned this pull request Dec 28, 2024

QKC/OP Devs Meeting #31 Agenda ethstorage/pm#163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`commitBlobTransaction`: fix nil pointer #23

`commitBlobTransaction`: fix nil pointer #23

blockchaindevsh commented Dec 26, 2024

syntrust Dec 27, 2024

blockchaindevsh Dec 27, 2024

syntrust Dec 27, 2024

qzhodl Dec 27, 2024

blockchaindevsh Dec 27, 2024 •

edited

Loading

qzhodl Dec 27, 2024

blockchaindevsh Dec 27, 2024

		@@ -396,7 +396,7 @@ func (miner Miner) commitBlobTransaction(env environment, tx *types.Transactio
		// isn't really a better place right now. The blob gas limit is checked at block validation time

-	// isn't really a better place right now. The blob gas limit is checked at block validation time
+	nblobs := 0
+	if sc == nil {
+		if env.isEffectivelySequencing() /* we want to allow blob tx without blobs when it's deriving */ {
+			panic("blob transaction without blobs in sequencing")
+		} else { // deriving, which does not have the sidecar
+			nblobs = len(tx.BlobHashes())
+		}
+	} else {
+		if !env.isEffectivelySequencing() {
+			panic("blob transaction with blobs in derivation")
+		}
+		nblobs = len(sc.Blobs)
+	}
+	// should we check tx.BlobHashes() matches that of sc.Blobs.BlobHashes()?
+	// Checking against blob gas limit: It's kind of ugly to perform this check here, but there
+	// isn't really a better place right now. The blob gas limit is checked at block validation time
+	// and not during execution. This means core.ApplyTransaction will not return an error if the
+	// tx has too many blobs. So we have to explicitly check it here.
+	if (env.blobs+nblobs)*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock {
+		return errors.New("max data blobs reached")
+	}

commitBlobTransaction: fix nil pointer #23

Are you sure you want to change the base?

commitBlobTransaction: fix nil pointer #23

Conversation

blockchaindevsh commented Dec 26, 2024

syntrust Dec 27, 2024

Choose a reason for hiding this comment

blockchaindevsh Dec 27, 2024

Choose a reason for hiding this comment

syntrust Dec 27, 2024

Choose a reason for hiding this comment

qzhodl Dec 27, 2024

Choose a reason for hiding this comment

blockchaindevsh Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

qzhodl Dec 27, 2024

Choose a reason for hiding this comment

blockchaindevsh Dec 27, 2024

Choose a reason for hiding this comment

`commitBlobTransaction`: fix nil pointer #23

`commitBlobTransaction`: fix nil pointer #23

blockchaindevsh Dec 27, 2024 •

edited

Loading