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

Dont append full wrapped blobtx to filters #9471

Merged
merged 4 commits into from
Feb 22, 2024
Merged

Dont append full wrapped blobtx to filters #9471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14ac921
Dont append full wrapped blobtx to filters
somnathb1 Feb 20, 2024
92a5247
Remove redundant var
somnathb1 Feb 20, 2024
96355cf
Fix slicing
somnathb1 Feb 21, 2024
3f76d8e
Rename var
somnathb1 Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions core/types/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,20 @@ func UnmarshalWrappedTransactionFromBinary(data []byte) (Transaction, error) {
return t, nil
}

// Remove everything but the payload body from the wrapper - this is not used, for reference only
func UnwrapTxPlayloadRlp(blobTxRlp []byte) (retRlp []byte, err error) {
if blobTxRlp[0] != BlobTxType {
return blobTxRlp, nil
}
it, err := rlp.NewListIterator(blobTxRlp[1:])
if err != nil {
return nil, err
}
it.Next()
retRlp = it.Value()
return
}

func MarshalTransactionsBinary(txs Transactions) ([][]byte, error) {
var err error
var buf bytes.Buffer
Expand Down
95 changes: 95 additions & 0 deletions core/types/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,16 @@ import (
"testing"
"time"

gokzg4844 "github.com/crate-crypto/go-kzg-4844"
"github.com/holiman/uint256"
"github.com/stretchr/testify/assert"

libcommon "github.com/ledgerwatch/erigon-lib/common"
"github.com/ledgerwatch/erigon-lib/common/fixedgas"
"github.com/ledgerwatch/erigon-lib/common/hexutility"
"github.com/ledgerwatch/erigon-lib/crypto/kzg"
"github.com/ledgerwatch/erigon-lib/txpool"
libtypes "github.com/ledgerwatch/erigon-lib/types"
types2 "github.com/ledgerwatch/erigon-lib/types"

"github.com/ledgerwatch/erigon/common"
Expand Down Expand Up @@ -816,3 +821,93 @@ func TestBlobTxEncodeDecode(t *testing.T) {
}
}
}

func makeBlobTxRlp() []byte {
bodyRlp := hexutility.MustDecodeHex(txpool.BodyRlpHex)

blobsRlpPrefix := hexutility.MustDecodeHex("fa040008")
blobRlpPrefix := hexutility.MustDecodeHex("ba020000")

var blob0, blob1 = gokzg4844.Blob{}, gokzg4844.Blob{}
copy(blob0[:], hexutility.MustDecodeHex(txpool.ValidBlob1Hex))
copy(blob1[:], hexutility.MustDecodeHex(txpool.ValidBlob2Hex))

var err error
proofsRlpPrefix := hexutility.MustDecodeHex("f862")
commitment0, _ := kzg.Ctx().BlobToKZGCommitment(blob0, 0)
commitment1, _ := kzg.Ctx().BlobToKZGCommitment(blob1, 0)

proof0, err := kzg.Ctx().ComputeBlobKZGProof(blob0, commitment0, 0)
if err != nil {
fmt.Println("error", err)
}
proof1, err := kzg.Ctx().ComputeBlobKZGProof(blob1, commitment1, 0)
if err != nil {
fmt.Println("error", err)
}

wrapperRlp := hexutility.MustDecodeHex("03fa0401fe")
wrapperRlp = append(wrapperRlp, bodyRlp...)
wrapperRlp = append(wrapperRlp, blobsRlpPrefix...)
wrapperRlp = append(wrapperRlp, blobRlpPrefix...)
wrapperRlp = append(wrapperRlp, blob0[:]...)
wrapperRlp = append(wrapperRlp, blobRlpPrefix...)
wrapperRlp = append(wrapperRlp, blob1[:]...)
wrapperRlp = append(wrapperRlp, proofsRlpPrefix...)
wrapperRlp = append(wrapperRlp, 0xb0)
wrapperRlp = append(wrapperRlp, commitment0[:]...)
wrapperRlp = append(wrapperRlp, 0xb0)
wrapperRlp = append(wrapperRlp, commitment1[:]...)
wrapperRlp = append(wrapperRlp, proofsRlpPrefix...)
wrapperRlp = append(wrapperRlp, 0xb0)
wrapperRlp = append(wrapperRlp, proof0[:]...)
wrapperRlp = append(wrapperRlp, 0xb0)
wrapperRlp = append(wrapperRlp, proof1[:]...)

return wrapperRlp
}

// This test is for reference
func TestShortUnwrap(t *testing.T) {
blobTxRlp := makeBlobTxRlp()
shortRlp, err := UnwrapTxPlayloadRlp(blobTxRlp)
if err != nil {
t.Errorf("short rlp stripping failed: %v", err)
return
}
prefixedRlp := append([]byte{0x03}, shortRlp...) // Added the 0x3 prefix for DecodeTransaction func
bbtx, err := DecodeTransaction(prefixedRlp)

if err != nil {
t.Errorf("short rlp decoding failed : %v", err)
}
wrappedBlobTx := BlobTxWrapper{}
err = wrappedBlobTx.DecodeRLP(rlp.NewStream(bytes.NewReader(blobTxRlp[1:]), 0))
if err != nil {
t.Errorf("long rlp decoding failed: %v", err)
}
assertEqual(bbtx, &wrappedBlobTx.Tx)
}

// This test actually applies to testing the behaviour as seen from the
// onNewTx behaviour in filters.go
func TestShortUnwrapLib(t *testing.T) {
blobTxRlp := makeBlobTxRlp()
shortRlp, err := libtypes.UnwrapTxPlayloadRlp(blobTxRlp)
if err != nil {
t.Errorf("short rlp stripping failed: %v", err)
return
}
blobTx, err := DecodeTransaction(shortRlp)

if err != nil {
t.Errorf("short rlp decoding failed : %v", err)
}
wrappedBlobTx := BlobTxWrapper{}
err = wrappedBlobTx.DecodeRLP(rlp.NewStream(bytes.NewReader(makeBlobTxRlp()[1:]), 0))
if err != nil {
t.Errorf("long rlp decoding failed: %v", err)
}

assertEqual(blobTx, &wrappedBlobTx.Tx)
}
7 changes: 6 additions & 1 deletion erigon-lib/txpool/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import (
"github.com/ledgerwatch/erigon-lib/metrics"
"github.com/ledgerwatch/erigon-lib/txpool/txpoolcfg"
"github.com/ledgerwatch/erigon-lib/types"
types2 "github.com/ledgerwatch/erigon-lib/types"
)

const DefaultBlockGasLimit = uint64(30000000)
Expand Down Expand Up @@ -1829,6 +1830,11 @@ func MainLoop(ctx context.Context, db kv.RwDB, p *TxPool, newTxs chan types.Anno
if len(slotRlp) == 0 {
continue
}
// Strip away blob wrapper, if applicable
slotRlp, err2 := types2.UnwrapTxPlayloadRlp(slotRlp)
if err2 != nil {
continue
}

// Empty rlp can happen if a transaction we want to broadcast has just been mined, for example
slotsRlp = append(slotsRlp, slotRlp)
Expand Down Expand Up @@ -1859,7 +1865,6 @@ func MainLoop(ctx context.Context, db kv.RwDB, p *TxPool, newTxs chan types.Anno
return
}
if newSlotsStreams != nil {
// TODO(eip-4844) What is this for? Is it OK to broadcast blob transactions?
newSlotsStreams.Broadcast(&proto_txpool.OnAddReply{RplTxs: slotsRlp}, p.logger)
}

Expand Down
16 changes: 4 additions & 12 deletions erigon-lib/txpool/pool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,23 +876,15 @@ func TestBlobTxReplacement(t *testing.T) {

// Todo, make the tx more realistic with good values
func makeBlobTx() types.TxSlot {
// Some arbitrary hardcoded example
bodyRlpHex := "f9012705078502540be4008506fc23ac008357b58494811a752c8cd697e3cb27" +
"279c330ed1ada745a8d7808204f7f872f85994de0b295669a9fd93d5f28d9ec85e40f4cb697b" +
"aef842a00000000000000000000000000000000000000000000000000000000000000003a000" +
"00000000000000000000000000000000000000000000000000000000000007d694bb9bc244d7" +
"98123fde783fcc1c72d3bb8c189413c07bf842a0c6bdd1de713471bd6cfa62dd8b5a5b42969e" +
"d09e26212d3377f3f8426d8ec210a08aaeccaf3873d07cef005aca28c39f8a9f8bdb1ec8d79f" +
"fc25afc0a4fa2ab73601a036b241b061a36a32ab7fe86c7aa9eb592dd59018cd0443adc09035" +
"90c16b02b0a05edcc541b4741c5cc6dd347c5ed9577ef293a62787b4510465fadbfe39ee4094"
bodyRlp := hexutility.MustDecodeHex(bodyRlpHex)

bodyRlp := hexutility.MustDecodeHex(BodyRlpHex)

blobsRlpPrefix := hexutility.MustDecodeHex("fa040008")
blobRlpPrefix := hexutility.MustDecodeHex("ba020000")

var blob0, blob1 = gokzg4844.Blob{}, gokzg4844.Blob{}
copy(blob0[:], hexutility.MustDecodeHex(validBlob1))
copy(blob1[:], hexutility.MustDecodeHex(validBlob2))
copy(blob0[:], hexutility.MustDecodeHex(ValidBlob1Hex))
copy(blob1[:], hexutility.MustDecodeHex(ValidBlob2Hex))

var err error
proofsRlpPrefix := hexutility.MustDecodeHex("f862")
Expand Down
15 changes: 13 additions & 2 deletions erigon-lib/txpool/testdata.go
View file
Open in desktop

Large diffs are not rendered by default.

26 changes: 26 additions & 0 deletions erigon-lib/types/txn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,3 +984,29 @@ func (al AccessList) StorageKeys() int {
}
return sum
}

// Removes everything but the payload body from blob tx and prepends 0x3 at the beginning - no copy
// Doesn't change non-blob tx
func UnwrapTxPlayloadRlp(blobTxRlp []byte) ([]byte, error) {
if blobTxRlp[0] != BlobTxType {
return blobTxRlp, nil
}
dataposPrev, _, isList, err := rlp.Prefix(blobTxRlp[1:], 0)
if err != nil || dataposPrev < 1 {
return nil, err
}
if !isList { // This is clearly not wrapped tx then
return blobTxRlp, nil
}

blobTxRlp = blobTxRlp[1:]
// Get to the wrapper list
datapos, datalen, err := rlp.List(blobTxRlp, dataposPrev)
if err != nil {
return nil, err
}
blobTxRlp = blobTxRlp[dataposPrev-1 : datapos+datalen] // Seek left an extra-bit
blobTxRlp[0] = 0x3
// Include the prefix part of the rlp
return blobTxRlp, nil
}
2 changes: 1 addition & 1 deletion turbo/rpchelper/filters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ func (ff *Filters) OnNewTx(reply *txpool.OnAddReply) {
if len(rlpTx) == 0 {
continue
}
txs[i], decodeErr = types.DecodeWrappedTransaction(rlpTx)
txs[i], decodeErr = types.DecodeTransaction(rlpTx)
Copy link
Member

@yperbasis yperbasis Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This change looks problematic to me. The unfortunate thing is that we use "wrapped" in two ways:
A) Whether a typed (such as EIP-1559 or blob) transaction, whose serialization starts with its type byte, is additionally wrapped into the RLP string.
B) Whether a blob transactions is "wrapped" to contain blobs/commitments/proofs.

While I agree that with this PR we need "unwrapped" in sense B here, we still need DecodeWrappedTransaction in sense A ­– otherwise EIP-1559 (type-2) transactions will fail I suspect.

I suggest adding a new parameter to UnmarshalWrappedTransactionFromBinary (and hence DecodeWrappedTransaction) that explicitly controls whether we expect type-3 (blob) transactions with (deserialized into BlobTxWrapper) or without (deserialized into BlobTx) blobs. Currently UnmarshalWrappedTransactionFromBinary implicitly does the former.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Qq: Wrapped in 2 ways? There is wrapped blobTx, and nothing else that is "wrapped" additionally

Your suggestion for the case B is wrapping of tx_payload and blobs, commitment, proofs into one RLP is what UnmarshalWrappedTransactionFromBinary deals with. The rest of the logic is the same.
If for any reason type-2 transactions are more wrapped, then the existing logic wouldn't have worked either.

Also here, OnNewTx will only process elements from txpool's reply. I am ensuring that the wrapped version of blobTx isn't fed into that.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I was confused by the comment of DecodeWrappedTransaction. I thought that DecodeWrappedTransaction differed from DecodeTransaction in how they treat RLP envelops of typed transactions. Take a look at TestEIP2718TransactionEncode, for example. What is called "binary" representation is its serialization prescribed by EIP-2718. It's used in some places, for example when calculating the transaction root of a block header. However, in other places (e.g. when sent over network in eth/68) this "binary" representation has to be additionally "wrapped" with an RLP prefix to make it a valid RLP. You can see in TestEIP2718TransactionEncode that the "RLP representation" is the same as the "binary representation" prefixed by b866. Note that the above is only a concern for typed transactions, not for legacy ones.

Anyway, it looks like in reality DecodeWrappedTransaction behaves the same as DecodeTransaction re. the RLP envelope.

if decodeErr != nil {
// ignoring what we can't unmarshal
ff.logger.Warn("OnNewTx rpc filters, unprocessable payload", "err", decodeErr, "data", hex.EncodeToString(rlpTx))
Expand Down
Loading