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

eth_getTransactionCount does not include pending transactions when 'pending' qualifier given #2880

Closed
drewshaver opened this issue Jul 31, 2016 · 49 comments

Comments

Projects
None yet
@drewshaver
Copy link

commented Jul 31, 2016

As the title states, tx count is not getting incremented for pending transactions when giving second argument of 'pending'. It seems to be treated the same as 'latest'.

System information

Geth version: 1.4.10
OS & Version: OSX

> web3.eth.getTransactionCount("0xf82e...", "pending");
5
> web3.eth.sendTransaction({from: "0xf82...", to: "0xf1c...", value: 42000000000000000});
I0624 15:57:41.848826 eth/api.go:1193] Tx(0xbd094a59eb8f05653f35fa93a9254db95bc6b9b5bdd3b95aedda27bb781545f9) to: 0xf1c...
"0xbd094a59eb8f05653f35fa93a9254db95bc6b9b5bdd3b95aedda27bb781545f9"
> web3.eth.getTransactionCount("0xf82...", "pending");
5 (should be 6)
> I0624 15:57:59.315075 miner/worker.go:337] 🔨  Mined block (#4529 / 7788e873). Wait 5 blocks for confirmation
I0624 15:57:59.315639 miner/worker.go:555] commit new work on block 4530 with 1 txs & 0 uncles. Took 529.458µs
I0624 15:57:59.315664 miner/worker.go:433] 🔨 🔗  Mined 5 blocks back: block #4524
I0624 15:57:59.315989 miner/worker.go:555] commit new work on block 4530 with 1 txs & 0 uncles. Took 310.783µs
> web3.eth.getTransactionCount("0xf82...", "pending");
6

@fjl fjl added this to the 1.5.0 milestone Aug 1, 2016

@fjl

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2016

This is critical for the upcoming Go API as well. Maybe getTransactionCount(..., "pending") should return the transaction pool's latest known nonce instead of looking at the pending block.

@bas-vk

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

Are you sure you used geth 1.4.10?

> eth.getTransactionCount(eth.accounts[0])
5
> eth.getTransactionCount(eth.accounts[0], "pending")
5
> eth.sendTransaction({from: eth.accounts[0], to: eth.accounts[1], value: 1})
"0xd3546397a3b0c1fb0a6c47364a66b1841f4ebffb15f58bb5c8d0e1f453108e59"
> eth.getTransactionCount(eth.accounts[0])
5
> eth.getTransactionCount(eth.accounts[0], "pending")
6
> miner.start(1)
true
... tx mined ...
> eth.getTransactionCount(eth.accounts[0])
6
> eth.getTransactionCount(eth.accounts[0], "pending")
6

Geth will grab the nonce from the pending state if you ask for it.

@murrekatt

This comment has been minimized.

Copy link

commented Aug 5, 2016

Same issue here. Looking at the code in eth/api.go here there is no mention of pending.

@bas-vk

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

stateAndBlockByNumber is called which gets the pending state from the miner.

@fjl

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2016

We will change it to look at the transaction pool because it knows about the latest nonce faster.

@murrekatt

This comment has been minimized.

Copy link

commented Aug 6, 2016

@fjl that sounds very good.

I'm testing out the Go bindings and in the example here there seems to be some races to be aware of due to the sleep:

address, tx, token, err := DeployToken(auth, backends.NewRPCBackend(conn), new(big.Int), "Contracts in Go!!!", 0, "Go!")
if err != nil {
    log.Fatalf("Failed to deploy new token contract: %v", err)
}
fmt.Printf("Contract pending deploy: 0x%x\n", address)
fmt.Printf("Transaction waiting to be mined: 0x%x\n\n", tx.Hash())

// Don't even wait, check its presence in the local pending state
time.Sleep(250 * time.Millisecond) // Allow it to be processed by the local node :P

name, err := token.Name(&bind.CallOpts{Pending: true})

I'm doing two transactions from the same account after each other and right now the nonce is the same for both which means once tx will be invalid once the other is mined.

How will the concurrency work after the change to use the transaction pool? Will multiple transactions from the same account increment the nonce without any tricks with sleep etc.?

Also when is this change planned to hit develop?

@fjl

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2016

I cannot give you a timeline but it will be included in the next major release.

@murrekatt

This comment has been minimized.

Copy link

commented Aug 6, 2016

OK, thanks @fjl.

I think the Go bindings are really cool and was also wondering a bit how they were meant to be used in various situations e.g.:

  1. single account calling multiple functions on a single contract (resulting in transactions)
  2. single account calling multiple contracts (resulting in transactions)

In addition, the two above could be done concurrently from e.g. different goroutines. As the underlying mechanism from the calling go code right now calls out to the backend over RPC to get the transaction count or later the tx pool nonce there is a race. Hence, I assume concurrent transactions for the same account is not supported. Right?

However, for 1 & 2 it would be possible to keep track of the nonce in the client code, maybe in the TransactOpts by the underlying backend? Or something along those lines so that a caller using the same account would automatically get the nonce incremented without a need to ask over RPC. This opens of course up other points such as keeping the local value in sync. Has this been thought about? I'd be interested to understand what's been considered.

@Nexus7

This comment has been minimized.

Copy link

commented Oct 26, 2016

@fjl : Just checking if this change to look at the transaction pool for the nonce was made yet as obviously this issue is still currently open here...

@nickluijtgaarden

This comment has been minimized.

Copy link

commented Mar 28, 2017

I am seeing that this issue is still open. Does this mean this is not fixed?

@doppio

This comment has been minimized.

Copy link

commented May 15, 2017

@fjl any update on this? Is this change still going to happen?

@mcdee

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2017

Any progress on this? It seems like a critical issue for those of us attempting to generate multiple transactions with Go, and still has a milestone tag of two major releases back.

@chatch

This comment has been minimized.

Copy link

commented Oct 24, 2017

There is a related issue in Parity (paritytech/parity-ethereum#5014) with "pending". Parity has a non-standard RPC call parity_nextNonce to get the nonce considering all pending transactions in the pool.

Maybe this could be added to the standard JSONRPC API. Something like eth_nextNonce. Or the suggestion on the issue of using "pending_mining" or "pending_inclusion" could work too.

Then this getTransactionCount with "pending" can remain as meaning "the currently mined block (including pending transactions)". ie. pending transactions in a block being mined.

@yondonfu

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

At the moment I'm doing some local testing with this change to GetTransactionCount in internal/ethapi/api.go to ask the transaction pool for the nonce if the the transaction count is asked for a pending block:

// GetTransactionCount returns the number of transactions the given address has sent for the given block number
func (s *PublicTransactionPoolAPI) GetTransactionCount(ctx context.Context, address common.Address, blockNr rpc.BlockNumber) (*hexutil.Uint64, error) {
	if blockNr == rpc.PendingBlockNumber {
		nonce, err := s.b.GetPoolNonce(ctx, address)
		if err != nil {
			return nil, err
		}

		return (*hexutil.Uint64)(&nonce), nil
	} else {
		state, _, err := s.b.StateAndHeaderByNumber(ctx, blockNr)
		if state == nil || err != nil {
			return nil, err
		}

		nonce := state.GetNonce(address)

		return (*hexutil.Uint64)(&nonce), nil
	}
}

Thoughts on if this is a reasonable solution?

This issue seems to still be a problem for anyone using abigen to submit concurrent transactions since transact in https://github.com/ethereum/go-ethereum/blob/master/accounts/abi/bind/base.go#L176 uses PendingNonceAt to retrieve the pending nonce which I believe uses GetTransactionCount

@davux

This comment has been minimized.

Copy link

commented Dec 22, 2017

@fjl (2016-08-06)

I cannot give you a timeline but it will be included in the next major release.

When is the next major release? It's been a year and a half and the bug is serious and still present.

@dshulyak

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

@yondonfu this is what @fjl suggested some time ago in this thread. In my opinion, you should submit PR with your change

@winteraz

This comment has been minimized.

Copy link

commented Jan 17, 2018

Any chance to have this fixed this year(2018) ?

kulerskim added a commit to elpassion/ethereum.rb that referenced this issue Feb 22, 2018

Allow to change nonce on contract before sending transaction
Reason: It allows a developer to calculate transaction nonce on his own.
May be needed when you want to control transactions order or calculate
nonce using transaction pool rather than relying on current client
algorithm.

Relates to:
- EthWorks#35
- ethereum/go-ethereum#2880

marekkirejczyk added a commit to EthWorks/ethereum.rb that referenced this issue Feb 27, 2018

Use pending transaction nonce as a default (#58)
* Include pending transactions in Client#get_nonce

Reason: Submitting more than one transaction from the same account
before the block is mined resulted with colliding transactions, because
of the same nonce. This change takes the nonce from the pending
transactions, so it's incrementes as the pool growths before block is
mined.

Relates to: #35

* Allow to change nonce on contract before sending transaction

Reason: It allows a developer to calculate transaction nonce on his own.
May be needed when you want to control transactions order or calculate
nonce using transaction pool rather than relying on current client
algorithm.

Relates to:
- #35
- ethereum/go-ethereum#2880
@vernon99

This comment has been minimized.

Copy link

commented Feb 28, 2018

I wonder, how can it still open? How people deal with a need to send multiple transactions from the same account without storing the nonce (super fragile and can lead to transaction failures or overrides)? Do I miss something or this is a must have for any normal project?

@gavinmarkglynn

This comment has been minimized.

Copy link

commented Mar 8, 2018

This seems like to be too serious of bug to be left hanging? Is there any possibility of a fix? This issue will cause stability problems for any general application using Ethereum smart contracts.

@vernon99

This comment has been minimized.

Copy link

commented Mar 8, 2018

Btw, for now it seems we found a workaround by accessing node's pool using txpool api, fetching pending transactions and adding the number to eth_getTransactionCount. But yeah, this is a super crappy hack since you have to do extra fetch of a massive size on any transaction. Should be fixed on a node level!

@drewshaver

This comment has been minimized.

Copy link
Author

commented Mar 9, 2018

@vernon99 just fyi, there's a potential race condition in that workaround if the node state changes between calls. (i think)

@enVolt

This comment has been minimized.

Copy link

commented Jun 27, 2018

A pending transaction is immediately relayed to peer nodes (You can see your transaction as Pending almost immediately on Etherscan).

So it should work fine. You can ask on their Gitter channel

@pstehlik

This comment has been minimized.

Copy link

commented Jun 27, 2018

@oakcookies @enVolt Even though the nonce would be relayed "quickly" it would not really support a situation where you want to load a lot of transactions at once. The different nodes behind the load balancer would have different states and you would quickly run into race conditions.
This would only work if you have a nonce in some kind of system like redis (like @flute says). Then put all the nodes behind the load balancer into a single system to keep the nonce on redis.
The general user-need is that they want to submit transactions in quick succession before they even hit the Ethereum network so some kind of shared state is required if you want to run multiple nodes in that (load balanced) setup.

@sgmheyhey

This comment has been minimized.

Copy link

commented Jul 3, 2018

$ geth version
Geth
Version: 1.8.11-stable
Git Commit: dea1ce052a10cd7d401a5c04f83f371a06fe293c
Architecture: amd64
Protocol Versions: [63 62]
Go Version: go1.10
Operating System: linux
> eth.getTransactionCount(eth.accounts[0], "pending")
10
> eth.getTransactionCount(eth.accounts[0])
6
> eth.getTransactionCount(eth.accounts[0], "pending")
10
> eth.getTransactionCount(eth.accounts[0])
10

Does it sometimes work? 8-o

@enVolt

This comment has been minimized.

Copy link

commented Aug 24, 2018

This is for ropsten testnet -

> admin.nodeInfo
{
  enode: "enode://df090c3d10b4e485e4e4f85eca81ed3acf7ba1f17a0e18d505aad84f4be4d044ffffac2a10d35791dd8b72a2d328bca5305ec7591e8aec98532ec485938318bf@0.0.0.0:30303",
  id: "df090c3d10b4e485e4e4f85eca81ed3acf7ba1f17a0e18d505aad84f4be4d044ffffac2a10d35791dd8b72a2d328bca5305ec7591e8aec98532ec485938318bf",
  ip: "0.0.0.0",
  listenAddr: "0.0.0.0:30303",
  name: "Geth/v1.8.8-stable-2688dab4/linux-amd64/go1.10.1",
  ports: {
    discovery: 30303,
    listener: 30303
  },
  protocols: {
    eth: {
      config: {
        byzantiumBlock: 1700000,
        chainId: 3,
        daoForkSupport: true,
        eip150Block: 0,
        eip150Hash: "0x41941023680923e0fe4d74a34bdac8141f2540e3ae90623718e47d66d1ca4a2d",
        eip155Block: 10,
        eip158Block: 10,
        ethash: {},
        homesteadBlock: 0
      },
      difficulty: 9696937350631166,
      genesis: "0x41941023680923e0fe4d74a34bdac8141f2540e3ae90623718e47d66d1ca4a2d",
      head: "0x52a714877fe45043a069597363c99a830d4460cb39e3ee2169a9f041be655250",
      network: 3
    }
  }
}
> eth.getTransaction("0xfe0878155927b58a77e8f32b3f2b360686e8897c6e45e5502a797f7251744753").from
"0x08f094d3bc043df21727919aad5a3a0d182fe32d"
> eth.getTransaction("0xfe0878155927b58a77e8f32b3f2b360686e8897c6e45e5502a797f7251744753").nonce
3462
> eth.getTransaction("0x708d1d0ba01b7ce82706bf7ffc03d14dbe768e518464644ef54b15f61eaeb956").nonce
3463
> eth.getTransaction("0x708d1d0ba01b7ce82706bf7ffc03d14dbe768e518464644ef54b15f61eaeb956").from
"0x08f094d3bc043df21727919aad5a3a0d182fe32d"
> eth.getTransactionCount('0x08f094d3bc043df21727919aad5a3a0d182fe32d')
3462
> eth.getTransactionCount('0x08f094d3bc043df21727919aad5a3a0d182fe32d', "pending")
3462

There are pending transaction, which are visible to my geth instance, but still getTransactionCount is not accounting these, when getting transactions with Pending status.

@ethereum ethereum deleted a comment from Saloha Jan 15, 2019

@ethereum ethereum deleted a comment from Saloha Jan 15, 2019

@ethereum ethereum deleted a comment from Saloha Jan 15, 2019

@fjl fjl modified the milestones: 1.5.0, 1.9.0 Jan 15, 2019

@fjl fjl removed the status:triage label Jan 15, 2019

@fjl

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

(Finally) fixed in #15794.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.