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

core/txpool: drop the zero balance #27833

Closed
wants to merge 1 commit into from

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Aug 1, 2023

As the tx.Cost() equals to

// Cost returns (gas * gasPrice) + (blobGas * blobGasPrice) + value.

But someone maybe sends a tx with zero gasPrice and zero value so that the later check will be passed

if balance.Cmp(cost) < 0 {

But after EIP1559, a base fee should be always be paid out, if the EOA's balance is zero, we don't need to add this tx into txpool.

Signed-off-by: jsvisa <delweng@gmail.com>
@jsvisa
Copy link
Contributor Author

jsvisa commented Aug 1, 2023

@jwasinger thanks for your reply, but they do happen, here is a simple reproduction script:

#!/usr/bin/env python

import argparse
import logging
from eth_account import Account
from web3 import Web3, HTTPProvider
import secrets


logging.basicConfig(
    format="[%(asctime)s] - %(levelname)s - %(message)s", level=logging.INFO
)


def main():
    parser = argparse.ArgumentParser(
        formatter_class=argparse.ArgumentDefaultsHelpFormatter,
        description="Send ethereum transaction",
    )
    parser.add_argument(
        "--provider",
        default="http://192.168.1.81:24545",
        help="Web3 provider",
    )
    parser.add_argument(
        "--count",
        type=int,
        default=10,
        help="count",
    )
    args = parser.parse_args()

    w3 = Web3(HTTPProvider(args.provider))

    priv = secrets.token_hex(32)
    private_key = "0x" + priv
    acct = Account.from_key(private_key)
    chain_id = w3.eth.chain_id
    address = acct.address

    logging.info("created new address: {}".format(address))

    logging.info(
        "before send tx, account's nonce: {}, balance: {}".format(
            w3.eth.get_transaction_count(address, "pending"),
            w3.eth.get_balance(address),
        )
    )
    for n in range(0, args.count):
        txn = dict(
            nonce=n,
            gasPrice=0,
            gas=100000,
            to=acct.address,
            value=0,
            chainId=chain_id,
        )
        signed_txn = w3.eth.account.sign_transaction(txn, private_key)

        txhash = w3.eth.send_raw_transaction(signed_txn.rawTransaction)
        logging.info(f"tx: {n} -> {txhash.hex()}")

    logging.info(
        "after send tx, account's nonce: {}, balance: {}".format(
            w3.eth.get_transaction_count(address, "pending"),
            w3.eth.get_balance(address),
        )
    )


if __name__ == "__main__":
    main()

execute the script:

./web3_tx.py --count 10

output

[2023-08-02 18:27:15,867] - INFO - created new address: 0xCA5065947E16Ce9f7326C3fb121F5964b062d2c2
[2023-08-02 18:27:16,026] - INFO - before send tx, account's nonce: 0, balance: 0
[2023-08-02 18:27:16,073] - INFO - tx: 0 -> 0x81b7d5792d6c0af133fb49679c11047766db1b0a6f0f7f587d374ada0a68c1ea
[2023-08-02 18:27:16,115] - INFO - tx: 1 -> 0x844b8465b83ce393e06211d114889e67b810c18bea19db4d391380eaf086a468
[2023-08-02 18:27:16,160] - INFO - tx: 2 -> 0x4c5398f38b51d1d7430e9c8b2ce6f86af28ecd094e9703be5d92815d123b9b51
[2023-08-02 18:27:16,200] - INFO - tx: 3 -> 0x83cc51ca38649eb5bf9d3b59e717f9ec5c49a1ba189df5f1517f647dec0f0bf5
[2023-08-02 18:27:16,238] - INFO - tx: 4 -> 0x12c0da0547570829ab42140c0e56c280465a7f9d8e43f2979b140af581329e5b
[2023-08-02 18:27:16,276] - INFO - tx: 5 -> 0xbb351b04c608f0d234f53b1aba414589db6653a0e71924a1415b44c85a582a9e
[2023-08-02 18:27:16,314] - INFO - tx: 6 -> 0x58765bc3e43e19348c9fe18ae68ffe2989daea9b6715124eebe985c0f7552d94
[2023-08-02 18:27:16,393] - INFO - tx: 7 -> 0xd4c9849664de4aaa86c4fcb9cd2d10c2343bc728ca8b6f214c79d67b353c3e25
[2023-08-02 18:27:16,440] - INFO - tx: 8 -> 0xfdf70a938822c9a983231706d803c2db05eef6e0a0ab5e1c9b8b44d729dff5bb
[2023-08-02 18:27:16,483] - INFO - tx: 9 -> 0xea4a6363aa7076e6687c459a1a0ee4bbe1b51bf6ae1f6a86cc1b7b2164246816
[2023-08-02 18:27:16,548] - INFO - after send tx, account's nonce: 10, balance: 0

@@ -208,6 +208,9 @@ func ValidateTransactionWithState(tx *types.Transaction, signer types.Signer, op
balance = opts.State.GetBalance(from)
cost = tx.Cost()
)
if balance.Sign() == 0 {
Copy link
Member

@rjl493456442 rjl493456442 Aug 2, 2023

Choose a reason for hiding this comment

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

I guess this additional clause can be incorrect if 1559 isn't activated in some private chains?

Personally I will prefer to not have it. It's more like a check from chain's perspective(basefee must cost something), but not from the txpool's perspective(enough funds to cover the expense).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. Let me see if I can check with eip1559 here or check it outside

Copy link
Contributor

Choose a reason for hiding this comment

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

Historically, it has been possible to send transactions from empty accounts, but we (geth) don't like it and have tried to prevent it (but it's been allowed by consensus). I think adding this check is in line with our previous stance, that we try to avoid it. It leads to unexpected quirks.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it's kind of impossible to set a 0 gas price on our pool, so the balance check in the next line will fail. I'm not a fan of adding checks for stuff that cannot happen really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karalabe but it did happen in the Goerli and Sepolia testnet. see the reproduce script in #27833 (comment)

and here is a simple sepolia docker-compose

version: '3'

services:
  el:
    image: ethereum/client-go:v1.12.0
    container_name: sepo-el
    stop_signal: SIGINT
    stop_grace_period: 2m
    deploy:
      resources:
        limits:
          memory: 8G
        reservations:
          memory: 2G
    healthcheck:
      test: ['CMD-SHELL', 'geth attach --exec eth.blockNumber /data/geth.ipc']
      interval: 10s
      timeout: 5s
      retries: 5
    command:
      - '--sepolia'
      - '--syncmode=snap'
      - '--db.engine=pebble'
      - '--datadir=/data'
      - '--datadir.ancient=/ancient'
      - '--cache.preimages'
      - '--cache.database=60'
      - '--cache.trie=30'
      - '--cache.gc=5'
      - '--cache.snapshot=5'
      - '--cache.trie.rejournal=48h'
      - '--maxpeers=100'
      - '--port=30303'
      - '--http'
      - '--http.addr=0.0.0.0'
      - '--http.port=4545'
      - '--http.api=engine,web3,eth,net,personal,miner,txpool,debug,admin'
      - '--ws'
      - '--ws.addr=0.0.0.0'
      - '--ws.port=4546'
      - '--ws.origins=*'
      - '--ws.api=engine,web3,eth,net,personal,miner,txpool,debug,admin'
      - '--graphql'
      - '--graphql.vhosts=*'
      - '--graphql.corsdomain=*'
      - '--authrpc.addr=0.0.0.0'
      - '--authrpc.port=8551'
      - '--authrpc.vhosts=*'
      - '--authrpc.jwtsecret=/jwt/jwt.hex'
      - '--http.vhosts=*'
      - '--http.corsdomain=*'
      - '--metrics'
      - '--metrics.expensive'
      - '--metrics.addr=0.0.0.0'
      - '--metrics.port=6040'
      - '--pprof'
      - '--pprof.addr=0.0.0.0'
      - '--pprof.port=6041'
      - '--txlookuplimit=0'
      - '--txpool.accountslots=32'
      - '--txpool.globalslots=1024'
      - '--txpool.accountqueue=256'
      - '--txpool.globalqueue=2048'
    volumes:
      - ./data:/data
      - /data/sepo-ancient:/ancient
      - ./jwt:/jwt
    restart: unless-stopped
    ports:
      - '5500:30303/tcp'
      - '5500:30303/udp'
      - '5545:4545'
      - '5546:4546'
      - '5040:6040'
      - '5041:6041'
      - '5551:8551'

  cl:
    image: prysmaticlabs/prysm-beacon-chain:v4.0.7
    container_name: sepo-cl
    stop_grace_period: 2m
    deploy:
      resources:
        limits:
          memory: 4G
        reservations:
          memory: 1G
    command:
      - '--accept-terms-of-use'
      - '--sepolia'
      - '--datadir=/data'
      - '--rpc-host=0.0.0.0'
      - '--rpc-port=9548'
      - '--p2p-tcp-port=13005'
      - '--p2p-udp-port=12005'
      - '--monitoring-host=0.0.0.0'
      - '--execution-endpoint=http://el:8551'
      - '--jwt-secret=/jwt/jwt.hex'
      - '--grpc-gateway-host=0.0.0.0'
      - '--grpc-gateway-port=3505'
      - '--grpc-gateway-corsdomain=*'
      - '--checkpoint-sync-url=https://sepolia.beaconstate.info'
      - '--genesis-beacon-api-url=https://sepolia.beaconstate.info'
    volumes:
      - ./prysm:/data
      - ./jwt:/jwt
    restart: unless-stopped

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it's kind of impossible to set a 0 gas price on our pool,

I guess that check is not performed for local transactions, @karalabe ?

@rjl493456442
Copy link
Member

With #27834, the 0 gasprice LOCAL transaction shouldn't be accepted after the london fork, right?

I guess this PR is not relevant anymore.

@holiman
Copy link
Contributor

holiman commented Sep 5, 2023

Triage discussion: we don't see the need for this check

@holiman holiman closed this Sep 5, 2023
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.

4 participants