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.

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

feat: parallel checktx #3

Merged
merged 38 commits into from
Jan 14, 2025
Merged

feat: parallel checktx #3

merged 38 commits into from
Jan 14, 2025

Conversation

dudong2
Copy link

@dudong2 dudong2 commented Dec 9, 2024

Applied prs


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@dudong2 dudong2 changed the title Feat/parallel checktx feat: parallel checktx Dec 9, 2024
@dudong2 dudong2 self-assigned this Dec 11, 2024
Copy link

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

This PR introduces a significant number of code and interface changes, but the description currently only includes a reference to related PRs. It feels like there’s a lack of explanation regarding the overall design choices and the rationale behind these interface changes.

While it might be challenging to cover every detail, it would be helpful to include a general overview of the design decisions and the reasons why certain interfaces needed to be modified. This would make it much easier for reviewers to understand the context and intent behind these changes.

@dudong2
The review is still ongoing. Is there any document or resource I can refer to for reviewing this PR?
Given the scope and complexity of the changes, having a detailed PR description or supporting documentation would be extremely helpful. The linked PRs provide some context, but since this is a new PR, a consolidated explanation of the code changes and their purpose would make the review process smoother.

abci/client/client.go Outdated Show resolved Hide resolved
mempool/mempool.go Show resolved Hide resolved
Copy link

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

The overall concept and logic looks great. However, I noticed quite a few compatibility issues. For instance, there are some cases where variables or function names differ from CometBFT even though they have similar meanings (e.g., CallbackGlobalCallback, NewReqRes(req *types.Request)NewReqRes(req *types.Request, cb ResponseCallback) etc.).

I believe it’s better to avoid changing variable and function names unnecessarily unless there’s a specific reason. Of course, this decision may depend on the purpose of this work.

It would be good to discuss with @dongsam whether the priority is to implement this feature quickly, with compatibility to be addressed later, or if we should focus on ensuring compatibility from the start.

If we decide to prioritize compatibility, I can provide more detailed comments.

@dongsam , could you clarify the objective and priority of this task? Should we focus on feature development for now, or should compatibility also be a key consideration?

Copy link

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

@dudong2
Most of these comments are related to compatibility with CometBFT.
While they may seem trivial, maintaining compatibility is important to ensure that future rebases or reviews by others—who may not be familiar with the original CometBFT logic or variables—can easily understand which parts of the code relate to CometBFT. Therefore, I believe aligning with CometBFT as much as possible would be beneficial.

abci/client/client.go Outdated Show resolved Hide resolved
abci/client/socket_client.go Outdated Show resolved Hide resolved
abci/types/application.go Show resolved Hide resolved
abci/client/client.go Outdated Show resolved Hide resolved
abci/client/client.go Outdated Show resolved Hide resolved
abci/client/client.go Outdated Show resolved Hide resolved
abci/client/grpc_client.go Outdated Show resolved Hide resolved
abci/client/grpc_client.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
@zsystm
Copy link

zsystm commented Jan 7, 2025

@dudong2

One more question. Did you test grpc client and socket client also?

If so, could you share how did you test that clients? I only tested in my local with local client.

@dudong2
Copy link
Author

dudong2 commented Jan 8, 2025

@zsystm
I haven't tested this with grpc or socket clients other than the local client. If I find a way to test it, I'll share it.

@dudong2 dudong2 force-pushed the feat/parallel-checktx branch from 88ce42e to 544741a Compare January 10, 2025 08:18
@dudong2 dudong2 requested a review from zsystm January 12, 2025 20:03
Copy link

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

@dudong2
Comments are based solely on code review; no tests have been conducted for this PR yet. Additional comments may follow after testing.

mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
mempool/clist_mempool.go Outdated Show resolved Hide resolved
abci/types/application.go Show resolved Hide resolved
@dudong2 dudong2 requested a review from zsystm January 13, 2025 06:39
@zsystm
Copy link

zsystm commented Jan 14, 2025

Testing note
I tested with this branch and checked it works well without problem.

testing binary info

test method
tps: 1,000 (duration: 2m 30s)
block size: 1,000

init node

init node with the following init.sh and consparams.sh 1000

# init.sh
KEY="mykey"
KEY1="mykey1"
CHAINID="basechain_2200-1"
MONIKER="localtestnet"
KEYRING="test"
KEYALGO="eth_secp256k1"
LOGLEVEL="info"
# to trace evm
#TRACE="--trace"
TRACE=""

# validate dependencies are installed
command -v jq > /dev/null 2>&1 || { echo >&2 "jq not installed. More info: https://stedolan.github.io/jq/download/"; exit 1; }

# Reinstall daemon
rm -rf ~/.basechaind*
make install

# Set client config
basechaind config set client chain-id $CHAINID
basechaind config set client keyring-backend $KEYRING

# if $KEY exists it should be deleted
basechaind keys add $KEY --keyring-backend $KEYRING --algo $KEYALGO
basechaind keys add $KEY1 --keyring-backend $KEYRING --algo $KEYALGO

# Set moniker and chain-id for basechain (Moniker can be anything, chain-id must be an integer)
basechaind init $MONIKER --chain-id $CHAINID

# Change parameter token denominations to abasecoin
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["staking"]["params"]["bond_denom"]="abasecoin"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["crisis"]["constant_fee"]["denom"]="abasecoin"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["coinswap"]["params"]["pool_creation_fee"]["denom"]="abasecoin"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["coinswap"]["standard_denom"]="abasecoin"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["gov"]["params"]["min_deposit"][0]["denom"]="abasecoin"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["gov"]["params"]["expedited_min_deposit"][0]["denom"]="abasecoin"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["evm"]["params"]["evm_denom"]="abasecoin"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["inflation"]["params"]["mint_denom"]="abasecoin"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json
cat $HOME/.basechaind/config/genesis.json | jq '.app_state["feemarket"]["params"]["no_base_fee"]=true' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json

# Set gas limit in genesis
cat $HOME/.basechaind/config/genesis.json | jq '.consensus["params"]["block"]["max_gas"]="10000000"' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json


# CUSTOM LOGS
sed -i '' 's/size = 10000/size = 1000000/g' $HOME/.basechaind/config/config.toml
sed -i '' 's/recheck = true/recheck = false/g' $HOME/.basechaind/config/config.toml
sed -i '' 's/timeout_commit = "5s"/timeout_commit = "1s"/' $HOME/.basechaind/config/config.toml

# disable produce empty block
if [[ "$OSTYPE" == "darwin"* ]]; then
    sed -i '' 's/create_empty_blocks = true/create_empty_blocks = false/g' $HOME/.basechaind/config/config.toml
  else
    sed -i 's/create_empty_blocks = true/create_empty_blocks = false/g' $HOME/.basechaind/config/config.toml
fi

# Allocate genesis accounts (cosmos formatted addresses)
basechaind add-genesis-account $KEY 100000000000000000000010000abasecoin --keyring-backend $KEYRING
basechaind add-genesis-account $KEY1 100000000000000000000000000abasecoin --keyring-backend $KEYRING

# Update total supply with claim values
validators_supply=$(cat $HOME/.basechaind/config/genesis.json | jq -r '.app_state["bank"]["supply"][0]["amount"]')
# Bc is required to add this big numbers
# total_supply=$(bc <<< "$amount_to_claim+$validators_supply")
total_supply=200000000000000000000010000
cat $HOME/.basechaind/config/genesis.json | jq -r --arg total_supply "$total_supply" '.app_state["bank"]["supply"][0]["amount"]=$total_supply' > $HOME/.basechaind/config/tmp_genesis.json && mv $HOME/.basechaind/config/tmp_genesis.json $HOME/.basechaind/config/genesis.json

# Sign genesis transaction
basechaind gentx $KEY 1000000000000000000000abasecoin --keyring-backend $KEYRING --chain-id $CHAINID

# Collect genesis tx
basechaind collect-gentxs

# Run this to ensure everything worked and that the genesis file is setup correctly
basechaind validate-genesis

if [[ $1 == "pending" ]]; then
  echo "pending mode is on, please wait for the first block committed."
fi

# Start the node (remove the --pruning=nothing flag if historical queries are not needed)
#basechaind start --pruning=nothing $TRACE --log_level $LOGLEVEL --minimum-gas-prices=0.0001abasecoin --json-rpc.api eth,txpool,personal,net,debug,web3 --rpc.laddr "tcp://0.0.0.0:26657" --api.enable --chain-id $CHAINID
# consparams.sh
if [ -z "$1" ]; then
  echo "Usage: $0 <N>"
  exit 1
fi

N=$1
MAX_GAS=$((N * 21000))

GEN_LOC=/Users/denver/.basechaind/config
GEN="$GEN_LOC/genesis.json"
TMP_GEN="$GEN_LOC/tmp_genesis.json"

jq --arg max_gas "$MAX_GAS" '.consensus.params.block.max_gas = $max_gas' $GEN > $TMP_GEN
mv -f $TMP_GEN $GEN

setup surge with the following config and run surge faucet: surge f

$HOME/.surge/config.toml
[faucet]
# Faucet configuration
mode = "offchain" # Faucet mode, e.g., "offchain" or "onchain"

[faucet.offchain]
chain_type = "ethermint" # Chain type, e.g., geth or ethermint
funding_amt = 1000000000000000000 # Funding amount per account (in wei)
bech32_prefix = "basechain" # Bech32 prefix for Cosmos-based chains
denom = "abasecoin" # Denomination for the chain
remote_genesis_loc = ""
ssh_key_loc = ""
genesis_loc = "/Users/denver/.basechaind/config/genesis.json" # Path to the genesis file
accounts_file_loc = "/Users/denver/.surge/offchain_accounts.json" # Path to the accounts for offchain mode

[faucet.onchain]
rich_private_keys_loc = "/Users/denver/.surge/rich_private_keys.json" # Path to rich accounts' private keys
accounts_file_loc = "/Users/denver/.surge/onchain_accounts.json" # Path to the accounts for onchain mode

[generator]
# Generator configuration
scenario = "stress" # Scenario: "stress", "spike", or "soak"
reuse_factor = 1 # Reuse factor for accounts, set to >1 to allow reuse
timeunit = "1s" # Time unit for transactions per unit
loads = [1000] # Transactions per unit for each phase
durations = ["2m30s"] # Durations for each phase
chain_id = 2200 # Chain ID for the blockchain
gas_limit = 21000 # Gas limit for transactions
gas_price = 30000000000 # Gas price (in wei)
send_amt = 1 # Amount to send in each transaction (in wei)
rest = "10s"
goroutine_burden = 20000
# Statistics configuration
extract_from=""
ssh_key_loc = ""
endpoint="http://127.0.0.1:26657"

[lb]
# Load balancer configuration
endpoints = [
  "http://127.0.0.1:8545" # RPC endpoint 1
]

[clients]
read_timeout = "5s"
write_timeout = "50ms"
max_idle_conn_duration = "100ms"
max_conns_per_host = 0
tcp_dialer_concurrency = 0
dns_cache_duration = "1h"

[chart]
start_time = "2025-01-14T05:32:28Z"
end_time = "2025-01-14T05:34:55Z"
custom_log_dir = "/Users/denver/customlogs"
image

start node: basechaind start --pruning=nothing --trace --log_level info --minimum-gas-prices=0.0001ausdt --json-rpc.api eth,txpool,personal,net,debug,web3 --json-rpc.address "0.0.0.0:8545" --rpc.laddr "tcp://0.0.0.0:26657" --api.enable true --chain-id basechain_2200-1 --cpu-profile cpu.profile > comet_consensus.log 2>&1
run surge: surge r

stop node after surge finished

copy comet_consensus.log to ~/customlogs: cp comet_consensus.log ~/customlogs

copy txs_per_time.csv to ~/customlogs: cp load-1000_timeunit-1s_dur-2m30s_timestamp-20250102T051459/txs_per_time.csv ~/customlogs

create tps underperformance chart: surge c thr

Copy link

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

LGTM (testing log: #3 (comment))

Notes

  • I didn't test grpc client and socket client because those are not used in most case.

@dudong2 dudong2 merged commit 2209f5c into basechain/develop Jan 14, 2025
15 of 16 checks passed
@dudong2 dudong2 deleted the feat/parallel-checktx branch January 14, 2025 06:55
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