Conversation
qzhodl
requested review from
blockchaindevsh,
iteyelmp,
ping-ke,
qizhou and
syntrust
| ); | ||
|
|
||
| function send(address to, uint32 destShard) external payable { | ||
| require(destShard != currentShard(), "use normal transfer on same shard"); |
There was a problem hiding this comment.
What if destShard doesn't exist? Also currentShard() is undefined.
There was a problem hiding this comment.
You're right that currentShard() shouldn't be there, and the EL should be shard-agnostic and not know which shard it's running on. The validity check (whether destShard is a valid shard ID, whether it equals the source shard, etc.) should live at the CL/master layer, not in the EL contract — same pattern as Ethereum staking, where the deposit contract doesn't validate validators; the consensus layer does.
That said, after discussing with Qi, I realized the pure staking-pattern approach has a deeper issue for QKC: if all xshard sends flow through the root chain (the way Ethereum deposits/withdrawals flow through the beacon chain), the root chain becomes a bandwidth hotspot. Ethereum can afford this because staking activity is low-volume (a few hundred deposits per day system-wide), but QKC's xshard could easily be tens of thousands of tx per second — that doesn't fit through a single root chain.
The current QKC design actually handles this correctly: xshard lists flow directly slave-to-slave (peer-to-peer between shards), and the root chain only commits a hash via the mheader. Root-chain bandwidth stays O(mheaders), independent of total xshard volume.
I'm writing a separate comparison doc, going through this trade-off in detail. Will follow up with concrete changes to §4.6 once that's done — likely the master should only coordinate metadata (which mheader, which root-block position, what's the xshard commit hash), not carry payload bytes.
|
|
||
| **Deterministic xshard ordering must be carefully engineered.** When master routes xshard deposits from a confirmed root block to destination shards, it must ensure deterministic ordering (e.g., by root block height, then mheader index, then `XshardSend.nonce`) so that independently-running shard CLs converge on identical deposit sequences. The cursor in block meta implicitly enforced this in the current design; the new design must enforce it explicitly in master. | ||
|
|
||
| **Root anchor is outside Engine API's model.** `hash_prev_root_block` has no geth counterpart. Encoding it into `extraData` works, but it means EL does not validate it — CL must. This is one more thing for CL to get right. |
There was a problem hiding this comment.
Maybe we can further add a simple assertion:
if len(header.Extra) != 32 {
return errors.New("extraData must be exactly 32 bytes (root anchor hash)")
}There was a problem hiding this comment.
Agree the check should exist, just put it in the CL header validator instead of geth.
| } | ||
|
|
||
| XshardDeposit { | ||
| from: Address (20 bytes) |
There was a problem hiding this comment.
Looks like the from field is never used on the destination side? The EL hook only does AddBalance(deposit.To, deposit.Value). If it's for event indexing or explorers, say so.
It helps to clarify: which fields are consensus-critical (needed for deterministic verification) vs. metadata (nice to have for tooling)? Making that distinction explicit would clarify why the
struct is shaped this way and whether the metadata fields belong in the ExecutionPayload or could live out-of-band.
|
|
||
| QKC defines its own versioned `ExecutionPayload` and `PayloadAttributes` by adding fields to the latest upstream versions — the same pattern Ethereum itself follows when it upgrades (V1 → V2 for Shanghai, V3 for Cancun, V4 for Pectra). The minor block is therefore not bit-for-bit identical to a mainnet Ethereum block; it is an Ethereum-compatible format with QKC extensions. This is intentional: the goal is to reuse geth's implementation (EVM, StateDB, tx pool, state rewind, etc.) and inherit its upgrades, not to make QKC blocks interchangeable with Ethereum mainnet blocks. | ||
|
|
||
| Since QKC has no `BeaconBlock`-like CL wrapper (there are no validator signatures or attestations to carry), the minor block itself is essentially this extended `ExecutionPayload` plus the PoW seal fields (which are already part of geth's header: `mixhash`, `nonce`, `difficulty`). |
There was a problem hiding this comment.
We can consider keeping BeaconBlock, putting the qkc-related properties into BeaconBlock, while keeping the existing geth block in the EL.
There was a problem hiding this comment.
The motivation is good but the wrapper alone doesn't avoid EL patches. CL-only metadata like hash_prev_root_block can cleanly live in a BeaconBlock (currently we use extraData — equivalent), but xshardDeposits / xshardSends must flow in/out of EL state regardless of how we wrap them at CL
There was a problem hiding this comment.
If xshardDeposits and xshardSends are included in the EL block body, should the document specify how they should be committed to the block hash?
|
|
||
| | Change | Reason | Migration path | | ||
| |---|---|---| | ||
| | Transaction format: QKC-specific tx → standard Ethereum typed tx (per-shard `chainId`) | Tx with extra fields is incompatible with geth's tx pool and signer | New genesis; users submit new transactions to the new chain; historic tx history not migrated | |
There was a problem hiding this comment.
Do you mean add chainId to TX? If so, QKC already have this.
There was a problem hiding this comment.
Not quite — the redesign isn't "adding chainId" (current QKC already encodes it in the upper 16 bits of FromFullShardKey/ToFullShardKey, plus a separate NetworkId). The change is replacing QKC's custom tx format (with NetworkId + FromFullShardKey + ToFullShardKey + GasTokenID + TransferTokenID + Version) with standard EIP-1559 typed tx where chainId is a single top-level field. Each shard gets a unique chainId, and the QKC-specific fields disappear. The result is that wallets / Solidity / explorers work natively without QKC adapters — same chainId concept, standard packaging.
| @@ -0,0 +1,994 @@ | |||
| # GoQuarkChain Rearchitecture Design Doc | |||
There was a problem hiding this comment.
Because it is a long document, a table of contents would be extremely useful in understanding the structure at a glance.
|
|
||
| A block hash rather than a block number is used as the state-of-reference because PoSW depends on which chain the miner is extending: at the same height there may be competing forks with different stake balances and different coinbase histories. This matches the convention of `eth_getBalance(addr, blockHash)` and every Engine API method that references a specific block. | ||
|
|
||
| #### Use of `extraData` for root anchor |
There was a problem hiding this comment.
It would be helpful to fully compare the EL block structure of the proposed QKC mblock to the Ethereum upstream in detail, including both new and repurposed fields.
|
|
||
| The redesign keeps the data-plane topology current QKC already has — source-shard payload pushed directly to destination shards — and replaces the EVM-integrated mechanics with two clean hooks at the EL boundary: a predeployed system contract on the source side (mirroring EIP-7002) and a pre-block system call on the destination side (a controlled generalization of EIP-4895 that supports value, calldata, and gas). The root chain still commits source mheaders for ordering, but **carries no xshard payload bytes** — master is control plane only. | ||
|
|
||
| The EVM proper sees no shard concept anywhere. All shard-awareness lives in (a) the source system contract's tx interface, (b) the EL's post-block `xshardSends` extraction, and (c) the EL's pre-block `xshardDeposits` apply hook. |
There was a problem hiding this comment.
But at least 'xshardDeposits' would be in the EL mblock, correct?
|
|
||
| QKC defines its own versioned `ExecutionPayload` and `PayloadAttributes` by adding fields to the latest upstream versions — the same pattern Ethereum itself follows when it upgrades (V1 → V2 for Shanghai, V3 for Cancun, V4 for Pectra). The minor block is therefore not bit-for-bit identical to a mainnet Ethereum block; it is an Ethereum-compatible format with QKC extensions. This is intentional: the goal is to reuse geth's implementation (EVM, StateDB, tx pool, state rewind, etc.) and inherit its upgrades, not to make QKC blocks interchangeable with Ethereum mainnet blocks. | ||
|
|
||
| Since QKC has no `BeaconBlock`-like CL wrapper (there are no validator signatures or attestations to carry), the minor block itself is essentially this extended `ExecutionPayload` plus the PoW seal fields (which are already part of geth's header: `mixhash`, `nonce`, `difficulty`). |
There was a problem hiding this comment.
If xshardDeposits and xshardSends are included in the EL block body, should the document specify how they should be committed to the block hash?
| A full project plan is out of scope here. In broad phases: | ||
|
|
||
| - **Phase 1 — Shard CL prototype against stock geth.** A minimal CL that can drive a single-shard geth via Engine API for basic block production and sync. No xshard yet. Proves the Engine API integration model. | ||
| - **Phase 2 — Xshard via system contract and Engine API extensions.** Implement source/destination hooks in geth, extend Engine API, build CL-side xshard push transport (using master as forwarding hub) and the destination-side cursor logic. Prove the xshard protocol end to end. |
There was a problem hiding this comment.
Should 'using master as forwarding hub' be removed now that the source CL is pushing xshard payloads directly to destination CLs?
| // destination rate limiting. | ||
| if d.DestGasPrice < destBaseFee { | ||
| // deposit reverts; full reserved gas refunded per d.RefundRate | ||
| refundRevert(stateDB, d) |
There was a problem hiding this comment.
refundRevert is not defined?
Key question: where does the refund go? The value and reserved gas were burned on the source shard. If the deposit reverts on the destination, the refund must credit d.From on the destination shard (since
the source already burned it). The doc should specify this explicitly.
| require(destGasPrice >= 1, "destGasPrice must be > 0"); // ensures positive fee per deposit | ||
| // msg.value must cover both the transferred value and gasLimit * destGasPrice | ||
| uint256 reserved = uint256(gasLimit) * destGasPrice; | ||
| require(msg.value >= reserved, "msg.value insufficient for value + dest gas"); |
There was a problem hiding this comment.
The require only checks msg.value >= reserved (the gas part), not msg.value >= transferValue + reserved. This is probably intentional (value = whatever's above the gas reservation), but the
doc should make this explicit to avoid confusion.
|
|
||
| - **Value transfer to EOA** (`data == ∅`, `create == false`, `to` is an EOA). EIP-4895 fast path: balance credit + charge `MIN_XSHARD_DEPOSIT_GAS`. No EVM invocation. | ||
| - **Call into existing contract with calldata** (`data != ∅`, `create == false`). System-level CALL into `to` with `value`, `data`, and `gasLimit - MIN_XSHARD_DEPOSIT_GAS` of gas. | ||
| - **CREATE a new contract** (`create == true`). System-level deployment; destination address derived from `(from, from's source-shard nonce at the time of `XshardSend.send`)` via the standard Ethereum CREATE rule. |
There was a problem hiding this comment.
Does it mean CREATE(from, sender_account_nonce) or CREATE(from, xshard_nonce)?
|
|
||
| ### 6.3 Overall Assessment | ||
|
|
||
| The trade-off is heavily in favor of rearchitecture **if and only if regenesis is acceptable**. Without regenesis, the geth divergence is largely structural and cannot be eliminated. Given the current fork is frozen at 2018 geth and missing 7+ years of upstream improvements, the case is strong. |
There was a problem hiding this comment.
Because the entire design is based on regenesis, we may need to assess the cost and risks of regenesis before proceeding to design details, like defining the migration scope, such as balances only vs code+storage+nonce.
And, before deciding on the migration scope, we need to conduct a state census at the most recent mainnet height, which includes account count, contract/storage size, token holder distribution, and so on, to get a sense of the amount of work.
A reliable migration tool would also require effort. There are some experiences we can refer to, such as the OP Mainnet Bedrock migration, which emphasizes "reproducibility and verifiability," featuring open-source migration tools, weeks of rehearsals, shadow forks, audits, and witness files—enabling anyone to sync a legacy node and reproduce the migration.
In addition to technical/protocol correctness, the coordination with the community/miner/holder and their response would also be considered.
4 participants
No description provided.