Skip to content

qkc/types: add logs and receipts#34

Open
blockchaindevsh wants to merge 3 commits into
qkc-3-types-02-token-balancesfrom
qkc-3-types-03-logs-receipts
Open

qkc/types: add logs and receipts#34
blockchaindevsh wants to merge 3 commits into
qkc-3-types-02-token-balancesfrom
qkc-3-types-03-logs-receipts

Conversation

@blockchaindevsh

Copy link
Copy Markdown

3rd PR for QuarkChain/pm#155.

Comment thread qkc/types/bloom9.go
return hexutil.UnmarshalFixedText("Bloom", input, b[:])
}

func CreateBloom(receipts Receipts) Bloom {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we only keep the CreateBloom function and remove other Bloom related code since it is the same as geth

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in fa57d72 .

Comment thread qkc/types/log.go Outdated

// Log represents a contract log event. These events are generated by the LOG opcode and
// stored/indexed by the node.
type Log struct {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think the key design question is whether the Python master/slave serialization format requires us to introduce qkc/types.Log as a new general-purpose log model.

Modern geth already uses *core/types.Log throughout StateDB, EVM execution, receipt processing, filtering, indexing, and related APIs. If we replace it with qkc/types.Log, or propagate a parallel log model into these layers, we will need changes or conversions across many geth call sites. This would also increase the maintenance cost of future geth updates.

Reusing core/types.Log should not change the QKC receipt root: geth’s RLP encoding of an individual log contains only Address, Topics, and Data, which matches the log payload used by QKC receipt RLP. QKC can still keep its own receipt type for QKC-specific receipt fields such as contract_address and contract_full_shard_key.

The place where QKC does need a different representation is the Python master/slave protocol. Python expects the following qkc/serialize schema:

recipient, topics, data,
block_number, block_hash, tx_idx, tx_hash, log_idx

This schema differs from the fields and types in core/types.Log. However, a different cluster serialization format does not necessarily require a different execution-layer model. We can define serialization-only cluster types at the master/slave RPC boundary.

This is also how pyquarkchain is structured today. It has separate classes for the two roles:

quarkchain.evm.messages.Log / Receipt
    EVM and receipt-trie objects

quarkchain.core.Log / TransactionReceipt
    query and master/slave serialization objects

In MinorBlock.get_receipt(), Python reads an evm.messages.Receipt, converts each EVM log with Log.create_from_eth_log(), and returns a core.TransactionReceipt containing core.Log objects. The converted object is then used by the cluster RPC response.

We can mirror that boundary in Go:

qkc/types.Receipt
  Logs: []*core/types.Log
            |
            | toClusterTransactionReceipt()
            v
ClusterTransactionReceipt
  Logs: []*ClusterLog
            |
            | qkc/serialize
            v
Python master

The cluster types could look approximately like:

type ClusterLog struct {
    Recipient   common.Address
    Topics      []common.Hash
    Data        []byte `bytesizeofslicelen:"4"`
    BlockNumber uint64
    BlockHash   common.Hash
    TxIndex     uint32
    TxHash      common.Hash
    Index       uint32
}

type ClusterTransactionReceipt struct {
    Success         []byte
    GasUsed         uint64
    PrevGasUsed     uint64
    Bloom           coretypes.Bloom
    ContractAddress account.Address
    Logs            []*ClusterLog `bytesizeofslicelen:"4"`
}

Introducing only ClusterLog would not be sufficient because receipt serialization recursively serializes its Logs field. The cluster layer should convert the complete object graph before calling qkc/serialize:

internal QKC receipt
    -> convert every core/types.Log to ClusterLog
    -> construct ClusterTransactionReceipt
    -> construct GetTransactionReceiptResponse
    -> qkc/serialize

GetLogResponse would similarly convert its []*core/types.Log into []*ClusterLog.

This boundary also gives us an explicit place to:

  • Omit geth-only fields such as BlockTimestamp and Removed.
  • Convert geth’s uint indexes to the Python protocol’s uint32.
  • Check for index overflow.
  • Preserve Python’s exact field order and length-prefix rules.

There are also two concrete compatibility issues in the current implementation.

First, qkc/types.Log does not implement an explicit QKC Serialize method. qkc/serialize therefore encodes it through reflection, making the Go struct declaration order part of the wire protocol.

The current Go order is:

Recipient, Topics, Data, BlockNumber, TxHash, TxIndex, BlockHash, Index

Python expects:

recipient, topics, data, block_number, block_hash, tx_idx, tx_hash, log_idx

TxHash and BlockHash currently occupy the opposite wire positions, so the encoded bytes are not compatible with the Python master. A test can miss this if it uses the same value for both hashes.

Second, the current receiptSer starts with:

TxHash common.Hash

but Python core.TransactionReceipt.FIELDS does not contain tx_hash; its first field is success. The current receipt encoding therefore has an extra leading 32 bytes and is also incompatible with Python.

Could we keep core/types.Log as the internal log model and move the Python-specific receipt/log representation to explicitly named cluster types? The current receiptSer could be refactored into ClusterTransactionReceipt, with the extra TxHash removed and its Logs field changed to []*ClusterLog.

It would also be useful to add Python-generated golden-vector tests using different TxHash and BlockHash values, plus a complete TransactionReceipt vector, to verify byte-for-byte compatibility.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in 19db4b5 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants