feat(metrics): calculate the real pending tx

georgehao commented 1 month ago

1. Purpose or design rationale of this PR

https://www.notion.so/scrollzkp/monitoring-Show-the-count-of-executable-transactions-on-txpool-graph-85e468811d334eed88466bddd043f3ac

because some pending tx's gasFeeCap less than the current baseFee, so add a metrics to calculate the real pending tx

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

[ ] build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
[ ] ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
[ ] docs: Documentation-only changes
[x] feat: A new feature
[ ] fix: A bug fix
[ ] perf: A code change that improves performance
[ ] refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
[ ] style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
[ ] test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

[ ] This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
[x] Yes

4. Breaking change label

Does this PR have the breaking-change label?

[x] This PR is not a breaking change
[ ] Yes

jonastheis commented 1 month ago

Another option could be calling https://github.com/scroll-tech/go-ethereum/blob/develop/core/tx_pool.go#L505 periodically in a separate thread to not block/prolong txpool reorgs.

HAOYUatHZ commented 1 month ago

Another option could be calling https://github.com/scroll-tech/go-ethereum/blob/develop/core/tx_pool.go#L505 periodically in a separate thread to not block/prolong txpool reorgs.

Yeah I agree with this approach

omerfirmak commented 1 month ago

Another option could be calling https://github.com/scroll-tech/go-ethereum/blob/develop/core/tx_pool.go#L505 periodically in a separate thread to not block/prolong txpool reorgs.

Even if this is called in another thread, it will hold the txpool lock and potentially interfere with block building and/or tx pool reorgs.

georgehao commented 1 month ago

Another option could be calling https://github.com/scroll-tech/go-ethereum/blob/develop/core/tx_pool.go#L505 periodically in a separate thread to not block/prolong txpool reorgs.

Even if this is called in another thread, it will hold the txpool lock and potentially interfere with block building and/or tx pool reorgs.

maybe we can recover pending := w.eth.TxPool().PendingWithMax(false, w.config.MaxAccountsNum) to pending := w.eth.TxPool().PendingWithMax(true, w.config.MaxAccountsNum) https://github.com/scroll-tech/go-ethereum/blob/4b85bbcbd8b3e7dfb2ce9562933c72c55f1319da/miner/scroll_worker.go#L445

jonastheis commented 1 month ago

Counting it in the worker might not actually yield the correct number of pending transactions depending on how w.config.MaxAccountsNum is configured. Which makes it quite complex to assess whether the number is correct or not. Also imo it seems out of place as it is a txpool metric and not every node might be running a worker in the future.

Let's not over complicate things. Why not just start simple with this approach and add an additional metric to see how long it blocks the txpool. If it turns out to be significant or we feel it interferes with block building and/or tx pool reorgs, we can still go the route @omerfirmak suggested and you implemented in https://github.com/scroll-tech/go-ethereum/pull/983/commits/c5df443e4b640ff6d531f5365f6f3420b2b1d793

georgehao commented 1 month ago

w.config.MaxAccountsNum

w.config.MaxAccountsNum is worker.config.MaxAccountsNum = math.MaxInt

jonastheis commented 1 month ago

Yes, by default worker.config.MaxAccountsNum = math.MaxInt but it's configurable with --miner.maxaccountsnum flag. This makes the result rely on side effects that don't have anything to do with what we're trying to achieve, makes it unreliable and much harder to reason about/complicated.

Also --miner.maxaccountsnum is set here in deployment: https://github.com/scroll-tech/testnet/blob/f459a3e4e558f456f3e60ffad07db70b8d2c0ad0/core/l2geth/l2geth_run.sh#L150

georgehao commented 1 month ago

Yes, by default worker.config.MaxAccountsNum = math.MaxInt but it's configurable with --miner.maxaccountsnum flag. This makes the result rely on side effects that don't have anything to do with what we're trying to achieve, makes it unreliable and much harder to reason about/complicated.

Also --miner.maxaccountsnum is set here in deployment: https://github.com/scroll-tech/testnet/blob/f459a3e4e558f456f3e60ffad07db70b8d2c0ad0/core/l2geth/l2geth_run.sh#L150

you are right

georgehao commented 1 month ago

So I'll follow @jonastheis 's approach, add another commit

omerfirmak commented 4 weeks ago

can we add a metric on how long statsWithMinBaseFee takes to run? Since it might interfere with worker, it is good to know if it is holding the txpool lock for a long period.

georgehao commented 4 weeks ago

can we add a metric on how long statsWithMinBaseFee takes to run? Since it might interfere with worker, it is good to know if it is holding the txpool lock for a long period.

add cd5ed7a

scroll-tech / go-ethereum