api: Fetch pending state from txpool #15963

stevenroose · 2018-01-24T15:29:06Z

This commit changes the source of the state when asking for the
pending state. Instead of consulting the miner of the node, it uses
teh pending state of the transaction pool.

Two reasons for this change:

Introduction of the consensus package + BFT algorithms

With the introduction of the consensus package and the potential BFT
algorithms being built with it, the concept of the "pending block/state"
changes its definition. In some of these algorithms, pending block can
mean a block that is in the process of validation, but that can no
longer be updated. This conflicts with the traditional meaning of
"pending state", being the latest block plus all known pending
transactions.

Most notably, the Istanbul implementation has this problem. This also
results in the values for the pending state being different for a node
that has the miner enabled and the miner disabled.

General principle that the miner package should be disabled for
non-mining nodes

So considering the previous point, the use of the miner package for
something else than mining should be evaluated. I think it makes sense
to try working towards not using it at all when a node does not
participate in the mining/validating process.
Currently, the miner's pending state is used (a.o.) for executing new
incoming transactions (because the EVM needs a "current block" for the
global getters). I think this should be fairly easy to work around and
this is a first step.

stevenroose · 2018-01-24T15:31:17Z

This is the related Quorum (Istanbul BFT) issue that made this issue come up: Consensys/quorum#247

karalabe · 2018-01-24T15:48:34Z

This conflicts with the traditional meaning of
pending block, being the latest block plus all known pending
transactions.

There was never such definition. The pending block was always the next block containing as many transactions as fits is.

Also I don't see what this PR supposed to do. The transaction pool only tracks virtual nonces, but otherwise does not have a state db. You attempt to operate on it as if it was a true state database containing all transactions.

Also the reason we had StateAndHeaderByNumber and not StateByNumber and HeaderByNumber is because your chain can change between invoking the two (and it indeed does for highly active nodes serving lots of requests), so your change introduces a data race.

General principle that the miner package should be disabled for
non-mining nodes

The miner is the one creating the pending state. Whether your node tries to find PoW is irrelevant, but if it requires the pending state, it needs the same logic as mining itself. Also this part of the code is being reworked currently.

stevenroose · 2018-01-24T16:08:20Z

The rationale of the specific issue can be found here: ethereum/EIPs#650 (comment)

Namely, all applications that rely on abigen rely on PendingNonceAt (aka eth_getTransactionCount) to sign new transactions. For these transactions to be valid, the correct nonce should be returned (i.e. the one taking all pending txs for the account in consideration). The implementation of eth_getTransactionCount uses the StateByNumber implementation changed in this PR.

There was never such definition. The pending block was always the next block containing as many transactions as fits is.

Regarding that, let me rephrase. PendingNonceAt talks about the "pending state", not really "pending block" as such. I don't know where the concept "pending state" is defined, but I always interpreted that as latest + pending txs.

I did realize the issue of getting the state and header from different sources. Only one method does that, though: doCall which executes tx code and needs to build a header.

I would suggest following this up with a change that totally kills the miner when --mine is not set, which I think makes sense. This would mean that we need some source for a new header on top of the latest, but that should not be that hard.

As an alternative to this PR, we could introduce a different endpoint in the API backend that is purely used for accessing "an account's next nonce", i.e. using the txpool for that.

stevenroose · 2018-01-24T16:11:11Z

The miner is the one creating the pending state.

The miner just takes the pending state from the txpool (here) and filters the result, f.e. to fill the block only up to the max gas limit.

This commit changes the source of the state when asking for the *pending* state. Instead of consulting the miner of the node, it uses teh pending state of the transaction pool. Two reasons for this change: 1. Introduction of the consensus package + BFT algorithms With the introduction of the consensus package and the potential BFT algorithms being built with it, the concept of the "pending block/state" changes its definition. In some of these algorithms, pending block can mean a block that is in the process of validation, but that can no longer be updated. This conflicts with the traditional meaning of "pending state", being the latest block plus all known pending transactions. Most notably, the Istanbul implementation has this problem. This also results in the values for the pending state being different for a node that has the miner enabled and the miner disabled. 2. General principle that the miner package should be disabled for non-mining nodes So considering the previous point, the use of the miner package for something else than mining should be evaluated. I think it makes sense to try working towards not using it at all when a node does not participate in the mining/validating process. Currently, the miner's pending state is used (a.o.) for executing new incoming transactions (because the EVM needs a "current block" for the global getters). I think this should be fairly easy to work around and this is a first step.

holiman · 2018-01-25T07:51:52Z

I think the thing 'closest' to what you want, is an additional api-method which would be "give me the nonce that the transaction-pending-pool considers the next available nonce for account x". This could be added to the txpool namespace imo.

Note, though, that relying on such a method to provide nonces in concurrent requests would be dangerous, since there would always be gaps between getting it and submitting a signed transaction into the pending-pool.

stevenroose · 2018-01-25T09:24:43Z

I think the thing 'closest' to what you want, is an additional api-method which would be "give me the nonce that the transaction-pending-pool considers the next available nonce for account x". This could be added to the txpool namespace imo.

Considering that abigen and bind, both part of this repo, are using ethclient.PendingNonceAt exactly for that use case, I assumed that that method was supposed to do what you said. The bind package literally uses that method to decide what nonce to use for its next transaction.

So either the bind package is broken because it uses the pending block instead of the pending state which potentially returns a sequence number that is already used, or either that method is ambiguous like I assumed.

So yes, if a NextNonceFor(account) could be part of the txpool namespace API (could be accounts as well if that exists), I'd be glad to contribute as much as I can. Perhaps you could point me to where the "txpool api namespace" lives, I can go from there. I'll also update bind and abigen to use the new method.

stevenroose · 2018-01-25T09:28:53Z

Apparently, there is Backend.GetPoolNonce which is not exposed but could be easily exposed for this purpose.

stevenroose · 2018-01-25T09:45:56Z

If I may quote. This literally confirms how I interpreted PendingNonceAt as opposed to @karalabe's interpretation.

// A PendingStateReader provides access to the pending state, which is the result of all
// known executable transactions which have not yet been included in the blockchain. It is
// commonly used to display the result of ’unconfirmed’ actions (e.g. wallet value
// transfers) initiated by the user. The PendingNonceAt operation is a good way to
// retrieve the next available transaction nonce for a specific account.
type PendingStateReader interface {
	PendingBalanceAt(ctx context.Context, account common.Address) (*big.Int, error)
	PendingStorageAt(ctx context.Context, account common.Address, key common.Hash) ([]byte, error)
	PendingCodeAt(ctx context.Context, account common.Address) ([]byte, error)
	PendingNonceAt(ctx context.Context, account common.Address) (uint64, error)
	PendingTransactionCount(ctx context.Context) (uint, error)
}

~ PendingStateReader interface

// The transaction must be signed and have a valid nonce to be included. Consumers of the
// API can use package accounts to maintain local private keys and need can retrieve the
// next available nonce using PendingNonceAt.

~ TransactionSender interface

holiman · 2018-01-25T10:06:09Z

or either that method is ambiguous like I assumed.

Oh most definitely. You're not the first to stumble upon that, it's been the cause of many bug reports.

So, I know there's been talk about removing the pending state in it's current form -- since it's kind of useless for anyone not mining; and most callers actually care about all the pending transactions, not just the ones who happen to be selected for inclusion in a potential next block.

I agree that the documentation you find is erroneous, and misleading. @karalabe @fjl what would you propose to be the right path forward to make our API less confusing and error-prone?

stevenroose · 2018-01-25T10:16:03Z

I would think a miner-independent "pending state" (as opposed to "pending block") would be ideal.

All the global variables the EVM exposes for the current block can be "dummied" by just creating a dummy header and use that header for all pending txs. (Difficulty will be the hardest, but can be extracted from some block validation logic, I suppose.)

phongtt3 · 2018-07-27T09:02:38Z

Can you pls port this PR to quorum and i can test? Currently i try to modify code in quorum, but build was failed because the param in quorum and go-ethereum is difference

updated: i ported to quorum, running -> the result is not good, we still can't send parallel transactions

karalabe · 2019-01-10T09:43:33Z

internal/ethapi/api.go

+	header, err := s.b.HeaderByNumber(ctx, blockNr)
+	if header == nil || err != nil {
+		return nil, 0, false, err
+	}


This is racey. The whole reason behind retrieving the header and the state at the same time is because the pending state can and will change very frequently, often between these two calls (retrieving the state and the header separately). This was our original solution too btw that we needed to fix, took quite a lot to detect and figure out the issue.

karalabe · 2019-01-10T09:56:51Z

eth/api_backend.go

 	// Pending state is only known by the miner
 	if blockNr == rpc.PendingBlockNumber {
-		block, state := b.eth.miner.Pending()
-		return state, block.Header(), nil
+		return b.eth.TxPool().State().StateDB, nil


AFAIK the transaction pool does not have a meaningful state, it just tracks pending nonces. Won't this break everything?

karalabe · 2019-01-10T09:58:57Z

I don't really think this PR makes sense in it's current form. We updated the nonce retrieval in 1.9.0 to retrieve the nonce from the pool, so that's fixed. The txpool does not have a state otherwise, so this PR doesn't do what you think it does.

karalabe · 2019-01-10T10:00:37Z

The issue is that the txpool's state is just a fake thing to track nonces, it doesn't track any real state (balances, contracts, whatnot). I could imagine some consensus engine needs perhaps some specifics, but we should discuss those in the context of relevant PRs, not standalone, because it's hard to follow.

stevenroose · 2019-01-10T10:02:00Z

I don't really think this PR makes sense in it's current form. We updated the nonce retrieval in 1.9.0 to retrieve the nonce from the pool, so that's fixed. The txpool does not have a state otherwise, so this PR doesn't do what you think it does.

I think that was the major problem we were having at the time, so that seems to be a good fix.

stevenroose requested review from holiman and karalabe as code owners January 24, 2018 15:29

stevenroose mentioned this pull request Jan 24, 2018

RPC call for account nonce does not update for pending transactions Consensys/quorum#247

Closed

stevenroose force-pushed the pending branch from a741fec to cff5803 Compare January 24, 2018 16:13

karalabe reviewed Jan 10, 2019

View reviewed changes

adamschmideg added the need:more-information label Jan 10, 2019

karalabe reviewed Jan 10, 2019

View reviewed changes

adamschmideg removed the need:more-information label Jan 10, 2019

karalabe closed this Jan 10, 2019

stevenroose deleted the pending branch January 10, 2019 10:02

JekaMas mentioned this pull request May 15, 2019

Breaking the concept of "pending block" autonity/autonity#109

Closed

zhuang-weiming mentioned this pull request Jul 6, 2019

ValueError: max() arg is an empty sequence if there is only 1 filled block drandreaskrueger/chainhammer#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Fetch pending state from txpool #15963

api: Fetch pending state from txpool #15963

stevenroose commented Jan 24, 2018 •

edited

Loading

stevenroose commented Jan 24, 2018

karalabe commented Jan 24, 2018

stevenroose commented Jan 24, 2018

stevenroose commented Jan 24, 2018 •

edited

Loading

holiman commented Jan 25, 2018

stevenroose commented Jan 25, 2018

stevenroose commented Jan 25, 2018

stevenroose commented Jan 25, 2018

holiman commented Jan 25, 2018

stevenroose commented Jan 25, 2018

phongtt3 commented Jul 27, 2018 •

edited

Loading

karalabe Jan 10, 2019

karalabe Jan 10, 2019

karalabe commented Jan 10, 2019

karalabe commented Jan 10, 2019 •

edited

Loading

stevenroose commented Jan 10, 2019

api: Fetch pending state from txpool #15963

api: Fetch pending state from txpool #15963

Conversation

stevenroose commented Jan 24, 2018 • edited Loading

stevenroose commented Jan 24, 2018

karalabe commented Jan 24, 2018

stevenroose commented Jan 24, 2018

stevenroose commented Jan 24, 2018 • edited Loading

holiman commented Jan 25, 2018

stevenroose commented Jan 25, 2018

stevenroose commented Jan 25, 2018

stevenroose commented Jan 25, 2018

holiman commented Jan 25, 2018

stevenroose commented Jan 25, 2018

phongtt3 commented Jul 27, 2018 • edited Loading

karalabe Jan 10, 2019

Choose a reason for hiding this comment

karalabe Jan 10, 2019

Choose a reason for hiding this comment

karalabe commented Jan 10, 2019

karalabe commented Jan 10, 2019 • edited Loading

stevenroose commented Jan 10, 2019

stevenroose commented Jan 24, 2018 •

edited

Loading

stevenroose commented Jan 24, 2018 •

edited

Loading

phongtt3 commented Jul 27, 2018 •

edited

Loading

karalabe commented Jan 10, 2019 •

edited

Loading