API for re-sending transactions

pipermerriam commented 6 years ago

What was wrong?

There are a few legitimate use cases for wanting to re-submit a transaction that has already been sent with modified parameters.

Resending the same transaction with a higher gas price during times of network congestion.
Quickly overriding a transaction sent in error (also by using a higher gas price)
In a case where you know there is a signed transaction that was sent but never included in a block, ensuring that it will not be applied at some point in the future.

How can it be fixed?

Implement a new API web3.eth.overrideTransaction(txn_hash, txn_overrides)

txn_hash is the hash of the transaction you wish to override.
txn_overrides is a dictionary of transaction parameters which should be used in place of the existing transaction parameters.

Behavior is as follows.

if no transaction can be found with the given hash an exception is thrown.
if the transaction has already been mined an exception is thrown.
The txn_overrides may contain any/all fields that are normally accepted as transaction params except nonce.
If gasPrice is included in the txn_overrides, it must be greater than the existing transaction gas price (TODO: should we allow a way to bypass this validation)
if gasPrice is not included in the txn_overrides we should ???? (TODO: how do we choose an appropriate gas price?).

pipermerriam commented 6 years ago

This is just a first stab at an API for this. It almost definitely can be improved.

carver commented 6 years ago

Other name options:

w3.eth.replaceTransaction()
w3.eth.resendTransaction()

Closes #495

carver commented 6 years ago

Split API in two

I like the idea of splitting this into two APIs:

w3.eth.overrideTransaction(txn_hash, new_transaction_dict) -- Use only the new transaction fields, do not reuse any values from the the current pending transaction
w3.eth.modifyTransaction(txn_hash, **individual_fields_to_replace) -- Use the keyword args provided, infer gasPrice if not provided, and then fill in unspecified values from the current pending transaction

Examples:

from web3.auto import w3

txn_b = {'from': w3.eth.accounts[0], 'to': w3.eth.accounts[1], value: 0, gas: 21000}
txn_b_hash = w3.eth.overrideTransaction(txn_a_hash, txn_b)

new_txn_hash = w3.eth.modifyTransaction(txn_hash, gasPrice=w3.toWei('5.5', 'gwei'))

Gas price inference

if gasPrice is not included in the txn_overrides we should ???? (TODO: how do we choose an appropriate gas price?).

Proposal -- get two values, and use the maximum price of the two:

The gas price of the pending transaction times 1.1 (the minimum required to propagate)
The gas price as calculated by the current gas price engine

pipermerriam commented 6 years ago

Lets use the API from @carver 's latest comment: https://github.com/ethereum/web3.py/issues/492#issuecomment-354572411

gitcoinbot commented 6 years ago

This issue now has a funding of 0.25 ETH (254.76 USD) attached to it.

If you would like to work on this issue you can claim it here.
If you've completed this issue and want to claim the bounty you can do so here
Questions? Get help on the Gitcoin Slack
$13838.61 more Funded OSS Work Available at: https://gitcoin.co/explorer

owocki commented 6 years ago

@vs77bb -- we have enough python talent in the gitcoin ecosystem we should be able to help get this fulfilled :)

vs77bb commented 6 years ago

@owocki agree :) will pub this one out!

monokh commented 6 years ago

Looks like a fun one 🙂 I'd give this a go but you're probably sick of seeing me around here by now 😄 I'll give it a little while to see if anyone else is keen otherwise happy to help

carver commented 6 years ago

you're probably sick of seeing me around here by now

lol, not at all! :D

I'll give it a little while to see if anyone else is keen

That's very polite of you, but not necessary IMO.

otherwise happy to help

A day seems like plenty of time to wait, if you decide to.

amites commented 6 years ago

starting to look into this one, if anyone has capacity to tackle it please post here else I'll likely start writing code Monday

pipermerriam commented 6 years ago

Lets make sure that whoever takes this on posts that they are starting work on it so that multiple people don't show up with solutions and make claiming the bounty contentious.

vs77bb commented 6 years ago

Good thought @pipermerriam. @amites Are you planning on claiming today? cc @owocki

amites commented 6 years ago

caught up on another project today, will claim tomorrow/wednesday once i've finished unless someone else wants to take it on before I get to it

carver commented 6 years ago

Sounds like we're in a race between who is the first one able to allocate time to work on this, between @amites and @monokh . Looks like both of you are happy to cede to the other, whoever gets the time first. Just be sure to claim it before working on it. :)

monokh commented 6 years ago

Ok, in this case I'll pick it up then. Claim incoming.

monokh commented 6 years ago

@owocki i've claimed this but gitcoin hasn't picked it up. I believe it has something to do with me resending the transaction with a higher gas price. Gitcoin might be tracking a specific tx hash and therefore doesn't know if another tx went through? Old transaction still running: https://etherscan.io/tx/0xc9b75d10184f59d474bd019ec3ecdc47f8763869486301750b3808b8cee5f48b New transaction that confirmed: https://etherscan.io/tx/0xe014e218f4827bbc68e48358a4eb935d195b10b5a58e896ed990f1879cf164cf

monokh commented 6 years ago

w3.eth.overrideTransaction(txn_hash, new_transaction_dict)

w3.eth.modifyTransaction(txn_hash, **individual_fields_to_replace)

Is there a reason one takes a dictionary and another takes an argument list? I can understand if it's the semantics of reading modify transaction x=1 y=2 compared to modify transaction {x: 1, y: 2} but on first glance it felt inconsistent

What do we feel about w3.eth.overrideTransaction vs w3.eth.replaceTransaction? I feel like w3.eth.overrideTransaction might give the impression that could override parts of the transaction. Is it just my english? 😄

If gasPrice is included in the txn_overrides, it must be greater than the existing transaction gas price (TODO: should we allow a way to bypass this validation)

Is this some limitation in how the nodes function? Otherwise I feel like it's too opinionated to have a check for something like that. I can't think of one but there could be a reason to decrease the gas price.

Proposal -- get two values, and use the maximum price of the two: The gas price of the pending transaction times 1.1 (the minimum required to propagate) The gas price as calculated by the current gas price engine

@carver could you clarify what the purpose of this kind of logic is? What would be the consequence of performing the same gasPrice defaulting that sending a new transaction has? i might be confused

carver commented 6 years ago

Is there a reason one takes a dictionary and another takes an argument list?

Yes, I think it helps reinforce the semantic difference between the two: that modify reuses the values of the original transaction, and that override does not. You need to put everything you want in the new_transaction_dict. (except the nonce, which should raise an exception if someone specifies a different nonce than the one that txn_hash has).

What do we feel about w3.eth.overrideTransaction vs w3.eth.replaceTransaction? I feel like w3.eth.overrideTransaction might give the impression that only parts of the transaction could be overwritten. Is it just my english? :smile:

I don't think override implies that only part will change, but it is a more common usage to say to override part of something than to replace part of something. So I don't think override adds that bit of confusion, but I'm happy with replace as an alternative. It's also a little bit easier to spell.

Is this some limitation in how the nodes function? ... could you clarify what the purpose of this kind of logic is? What would be the consequence of performing the same gasPrice defaulting that sending a new transaction has?

Yes, once you have broadcast a transaction, the only effective way to replace it is with a transaction that has a higher gas price (by 10%, in most default implementations). Mining nodes will just ignore lower priced transaction broadcasts (why not, they get more fees for mining with your earlier broadcast transaction :smile: ).

gitcoinbot commented 6 years ago

The funding of 0.25 ETH (238.7 USD) attached has been claimed by @monokh.

@monokh, please leave a comment to let the funder and the other parties involved your implementation plan. If you don't leave a comment, the funder may expire your claim at their discretion.

Learn more on the gitcoin issue page
Questions? Get help on the Gitcoin Slack
$14321.91 more Funded OSS Work Available at: https://gitcoin.co/explorer

owocki commented 6 years ago

@monokh thx for the heads up, i just sync'd the issue from web3 => gitcoin

monokh commented 6 years ago

@carver thanks

Yes, once you have broadcast a transaction, the only effective way to replace it is with a transaction that has a higher gas price (by 10%, in most default implementations). Mining nodes will just ignore lower priced transaction broadcasts (why not, they get more fees for accepting your earlier broadcast transaction 😄 ).

That makes sense from a convenience perspective but I wonder if considering this in the API itself is necessary. For me, I would expect replacing a transaction to work as any other transaction, i.e. i provide the parameters and the node receives it. Now, if I provided a parameter that miners didn't like then as the consumer of the API, I should understand and handle that. That means that if miner expectations of these parameters change for whatever reason in the future then apps can adapt.

Hope i'm not being too pedantic here. I guess its a question of how much convenience vs flexibility this api wants to provide.

I'm just getting the project up and creating an example for myself to walk through. I should hopefully have something to show tomorrow 🙂

carver commented 6 years ago

That makes sense from a convenience perspective but I wonder if considering this in the API itself is necessary. For me, I would expect replacing a transaction to work as any other transaction, i.e. i provide the parameters and the node receives it. Now, if I provided a parameter that miners didn't like then as the consumer of the API, I should understand and handle that.

It goes further than this, because nodes won't relay the transaction if it doesn't have a gas price bump. Last I checked geth errors if you try to replace a transaction without giving it a 10% price bump. I could maybe be convinced that if someone hard-codes the gas into the replacement, that we can just let the node error out. But we absolutely need to give at least a 10% price bump when estimating the price for the user.

monokh commented 6 years ago

What is the behaviour when geth does error? Does the RPC call return something useful where we can relay back to the user the fact that their gas price didn't meet requirements? Something for me to have a look at.

I think its starting to click with me why it makes sense to provide the gas price estimation this way, given that the default gas price is the node's implementation anyway.

We still want the user to be able to override the gasPrice though right? maybe to increase it higher than 10% or for another reason. At that point it should be their responsibility the outcome of their parameters.

carver commented 6 years ago

We still want the user to be able to override the gasPrice though right?

Yes, for sure.

monokh commented 6 years ago

No codes today unfortunately but I have been exploring the implementation. Hit a bit of a roadblock though

There's a requirement here for doing the replacement based on an unmined transaction.

if no transaction can be found with the given hash an exception is thrown. if the transaction has already been mined an exception is thrown.

This makes sense as the checks and integration with the previous transaction is what makes it easy and useful as compared to just sending another transaction manually. However there seems to be a limitation on what can be gathered from an unmined transaction using its hash. eth_getTransactionByHash isn't able to return a submitted transaction until its mined. This is from my testing using testrpc and an infura node. Please correct me if i'm wrong here, I'm about to test with a parity node. But if this was the case, the only option then would be to watch a pending transaction filter running in the background which feels a little impractical.

Thoughts? I might be going down the wrong alley 😄

carver commented 6 years ago

However there seems to be a limitation on what can be gathered from an unmined transaction using its hash. eth_getTransactionByHash isn't able to return a submitted transaction until its mined.

Check out the json-rpc docs. That method is definitely intended to work for pending transactions.

For example:

blockHash: DATA, 32 Bytes - hash of the block where this transaction was in. null when its pending.

Just a guess: testrpc was set to auto-mine, so it's never pending when you ask for it. Infura doesn't keep a running list of pending transactions, so Infura just won't support this ability as well as a full node.

monokh commented 6 years ago

blockHash: DATA, 32 Bytes - hash of the block where this transaction was in. null when its pending.

Yea that's exactly what made me think initially that unmined transactions should be able to be retrieved with that rpc call.

Just a guess: testrpc was set to auto-mine, so it's never pending when you ask for it.

I did try configuring block time on testrpc to a certain # of seconds but wasn't able to see the pending transactions. I asked truffle people if ganache had some sort of bug and they are unsure. Issue to be raised later on with them to look into that.

I'll be trying a full node (parity) later on to see what it looks like there 🙂 🤞

monokh commented 6 years ago

Results are in. eth_getTransactionByHash returns the unmined transactions fine on full nodes. Probably worth considering that some nodes may not though in which case the API won't be usable... It's strange that Infura doesn't keep the tx pool, maybe we can convince that this is helpful?

Did I mention by testrpc i meant ganache-cli? 😄 Issue raised with them to look into it. With the the python testrpc, it seems that it doesn't support blocktimes and calling eth_getTransactionByHash just returns the immediately mined transaction which presents an issue for testing 🤔 for the tests required in the library and in general for development

Anyway, I can make a start on the implementation while following up these threads. |

Btw i found that there is a web3py gitter, I can catch you there instead of clogging up this topic.

carver commented 6 years ago

Btw i found that there is a web3py gitter, I can catch you there instead of clogging up this topic.

Yeah, come on over if there's something you think we can knock out faster on chat. But sometimes those conversations get lost, so it can be nice to have them grouped together under an issue. I'm totally happy to keep related discussion happening on this page, it wouldn't be "clogging up" the thread. :)

monokh commented 6 years ago

Web3.py actually uses eth-tester now which includes options for disabling auto mining. Therefore there won't be problems with the tests or development:

tester = EthereumTester(auto_mine_transactions=False)
web3 = Web3(EthereumTesterProvider(tester))
tx = web3.eth.sendTransaction({
    'from': '0x82A978B3f5962A5b0957d9ee9eEf472EE55B42F1',
    'to': '0x7d577a597B2742b498Cb5Cf0C26cDCD726d39E6e',
    'value': '1000'
})
print(web3.eth.getTransaction(tx.hex()))
>>> ...... 'nonce': 0, 'blockHash': None, 'blockNumber': None, ......

monokh commented 6 years ago

Put in a very beginning of work on this. Apologies that this is going slow. Not had much time and doing more learning than coding here 😄 Should be able to get something reviewable quality after the weekend.

vs77bb commented 6 years ago

Hi @monokh how are things going here? cc @owocki

monokh commented 6 years ago

Hi @vs77bb, sorry if it wasn't clear. The PR https://github.com/ethereum/web3.py/pull/586 is waiting on https://github.com/ethereum/eth-tester/pull/60 to be merged. Should be good after that - we are very close 🙂

nuliknol commented 6 years ago

How can you replace a transaction by using a hash? The hash is not constant, it changes when you modify any field of the transaction. The unique key for the transaction is not its hash, but address+nonce. So, your request to add such function to the api is nonsense. It is going to have many failures if you enqueue many transactions with future nonces, or replace a transaction quickly many times, during 'pending' period.

voith commented 6 years ago

@nuliknol looking at the implementation what you're saying doesn't seem correct. Please read this: https://github.com/ethereum/web3.py/pull/586/files#diff-df684340996e8b6d45d9f04f76673781R106. Also, do you have a test case to prove what you're saying?

pete00067 commented 4 years ago

Hello, I am testing w3.eth.modifyTransaction() on Kovan however it returns ValueError: Could not format value '0x2a' as field 'chainId' when I tried to replace the tx. I am using infura as node, does this only works with local node?

carver commented 4 years ago

Commenting on old, closed issues isn't the best way to get support. One excellent way to get support is https://ethereum.stackexchange.com . Sometimes our gitter channel works well too, but makes it harder for others to find the same answer you were looking for.

Wherever you post, you will get better & faster answers if your question has a minimal, complete, and verifiable example: https://stackoverflow.com/help/minimal-reproducible-example

ethereum / web3.py