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

Increasing of the block number for the confirmation workflow over HTTP is wrong. #2661

Closed
szerintedmi opened this issue Apr 8, 2019 · 21 comments · Fixed by #2703
Closed
Labels
Bug Addressing a bug

Comments

@szerintedmi
Copy link

szerintedmi commented Apr 8, 2019

Description

A tx never resolves on ganache with WebsocketProvider whatever I set transactionConfirmationBlocks to.
sendTransaction, sendSignedTransaction and confirmation events are NOT triggered.
Only transactionHash event is triggered.

NB:

  • I followed advice from closed Tx never resolves on ganache and no confirmation or receipt event triggered beta50-52 #2652 but it didn't help. If it works for others then it could be something trivial we do incorrectly but we tried it on multiple environments in different ways with no luck.
  • I tried the same script on different web3.js versions (only changing the WebsocketProvider constructor params) and it WORKs with beta48 but not with beta49-52.
  • It breaks a different way with HttpProvider - when transactionConfirmationBlocks set to 1 it resolves but anything higher it still never resolves.
  • I tried to call evm_mine manually from transactionHash event or call multiple .sendTransaction in a separate console but no effect.

Expected behavior

This should work on ganache as per documentation:

const Web3 = require("web3");

const OPTIONS = {
  defaultBlock: "latest",
  transactionConfirmationBlocks: 1,
  transactionBlockTimeout: 5
};

const web3 = new Web3(
  new Web3.providers.WebsocketProvider("ws://localhost:8545"),
  // Web3.providers.HtttpProvider("http://localhost:8545")
  null,
  OPTIONS
);

sendTx()
  .then(receipt => {
    console.log("Got receipt");
  })
  .catch(error => console.log("Got error:", error));

async function sendTx() {
  const accounts = await web3.eth.personal.getAccounts();

  const tx = web3.eth
    .sendTransaction({
      to: accounts[1],
      from: accounts[0],
      value: web3.utils.toWei("0.1", "ether")
    })
    .on("transactionHash", txHash => {
      // web3.currentProvider.send("evm_mine"); // execution of it seems to be blocked by sendTransaction in beta52
      console.log("on transactionHash", txHash);
    })
    .on("receipt", receipt => {
      console.log("on receipt");
    })
    .on("confirmation", (confirmationNumber, receipt) => {
      console.log("on confirmation", confirmationNumber);
    })
    .on("error", error => {
      console.log("on error", error);
    });

  const receipt = await tx;
  return receipt;
}

Actual behavior

  • txHash recevied
  • no receipt or confirmation event called
  • the tx never resolves

Output from the included gist with Websocket Provider

$ node ./sendTx.js 
WebsocketProvider version: 1.0.0-beta.52 web3.eth.transactionConfirmationBlocks: 1 web3.transactionConfirmationBlocks: 1
on transactionHash 0x3bc1bace44ba626b1b7c6138d9d144100ab96b7eb448de8723d1528491802dd4

And hanging forever...

Output from ganache (trimmed):

$ yarn ganache-cli
yarn run v1.13.0
$ [...]/node_modules/.bin/ganache-cli
Ganache CLI v6.4.2 (ganache-core: 2.5.4)
[...]

Gas Price
==================
20000000000

Gas Limit
==================
6721975

Listening on 127.0.0.1:8545
eth_accounts
eth_gasPrice
eth_sendTransaction

  Transaction: 0x2835ffae4a227eb1a789d1bad0fbb3501b6b31bf46c5935299a8d83ca82aa762
  Gas usage: 21000
  Block Number: 1
  Block Time: Mon Apr 08 2019 11:10:47 GMT+0100 (British Summer Time)

eth_subscribe

Steps to reproduce the behavior

run sendTx.js
with node ./txSend.js (while ganache is running)

Error Logs

N/A

Gists

Barebone gist to demonstrate the issue:
sendTx.js

Versions

  • web3.js: beta52
  • nodejs: 8.15.3
  • browser: N/A
  • ethereum node: ganache-cli v6.4.2
@nivida
Copy link
Contributor

nivida commented Apr 8, 2019

It breaks a different way with HttpProvider - when transactionConfirmationBlocks set to 1 it resolves but anything higher it still never resolves.

It does never resolve because ganache doesn't mine new blocks but it does fake the newHeads subscription which is the reason why it is working over WS.

Did you test it with a code for example like this:

function sendTx() {
        return new Promise(async (resolve, reject) => {
            const accounts = await web3.eth.personal.getAccounts();
            web3.eth.sendTransaction({
                to: accounts[1],
                from: accounts[0],
                value: web3.utils.toWei('0.1', 'ether')
            })
                .on('transactionHash', txHash => {
                    console.log('on transactionHash', txHash);
                })
                .on('receipt', resolve)
                .on('confirmation', (confirmationNumber, receipt) => {
                    console.log('on confirmation', confirmationNumber);
                })
                .on('error', reject);
        });
    }

// OR: 

async function sendTx() {
            const accounts = await web3.eth.personal.getAccounts();
            return web3.eth.sendTransaction({
                to: accounts[1],
                from: accounts[0],
                value: web3.utils.toWei('0.1', 'ether')
            });
    }

The problem you have is the combination of Promise and the EventEmitter in one call. We had an open issue because of "UnhandledPromiseRejection" errors when someone was just using the events of the PromiEvent and not the promise. This means it doesn't resolve or reject the Promise if an event listener for the error or receipt event exists.

@nivida nivida added the support label Apr 8, 2019
@princesinha19
Copy link
Contributor

princesinha19 commented Apr 8, 2019

@szerintedmi I think it's not working due to promises. try to replace promises to the callback. It could be a temporary solution.

const tx = web3.eth
    .sendTransaction({
      to: accounts[1],
      from: accounts[0],
      value: web3.utils.toWei("0.1", "ether")
    }, () => {
         
});

I tested on ganache and callback is working. I think this a problem of ganache.

In Geth it works fine both with promises and callback.

@nivida
Copy link
Contributor

nivida commented Apr 8, 2019

@princesinha19 I wouldn't recommend using callbacks because they will get marked as deprecated in the near future. :)

@princesinha19
Copy link
Contributor

Yes right, @nivida. That's why I stated it as a temporary solution for him.

@phraktle
Copy link

phraktle commented Apr 8, 2019

@nivida If callbacks are deprecated, how are you supposed to get a txhash from the API (without waiting for the receipt)?

@szerintedmi
Copy link
Author

szerintedmi commented Apr 8, 2019

@nivida : Thank you for the fast response

It seems there are multiple overlapping things here then:

  1. A tx never resolves (with await or .then()) if there is an .on("receipt") attached. If I remove the .on("receipt") then it resolves.
    If it's an intended behaviour then a mention in docs might be helpful for others.
  2. We don't receive confirmations from ganache on websocket. If I understand well ganache emulates confirmations via "fake" newHeads.
    I assume you don't plan to support it ? So should we change our tests to simulate block confirmations?
  3. On http it immediately receives 1 confirmation (unlike with websocket) but never receives more. It would be less confusing if websocket and http could work the same way.

Are there tickets for any of these or shall I create?

Btw, do you have any integration tests with ganache? I'm more than happy to contribute some simple ones.

@eduardonunesp
Copy link

@nivida
I think the same issue is happening to me and it's caused on https://github.com/ethereum/web3.js/blob/1.0/packages/web3-core-method/src/observers/TransactionObserver.js#L231, because the number is passed as a number, not a hex number, so it searches for a block that not exists

@eduardonunesp
Copy link

eduardonunesp commented Apr 8, 2019

So when the number arrives here: https://github.com/ethereum/web3.js/blob/1.0/packages/web3-core-method/src/observers/TransactionObserver.js#L282, it wrongly is parsed as base 16, but actually is a base 10. And after you likely to be stuck on the TransactionObserver by waiting 24 txs to be confirmed (default value I guess) to exit off the observer https://github.com/ethereum/web3.js/blob/1.0/packages/web3-core-method/src/observers/TransactionObserver.js#L166.

The problem on Ganache is that ganache doesn't mine by itself rather it mines a block when receiving a tx.

Ganache only mines if the user request it to mine curl -v -X POST --data '{"id":0,"jsonrpc":"2.0","method":"evm_mine","params":[]}' http://localhost:7545

I not sure if is a good idea to wait for 24 txs to consider the tx confirmed, it's getting stuck on the observer, maybe the user can provide a confirmation number (not checked the docs yet for it), on chains that have 1 tx per second is ok, but for mainnet is not cool. Does HTTP provider needs the confirmation like that ?

@nivida nivida added Bug Addressing a bug and removed support labels Apr 8, 2019
@nivida nivida changed the title sendTransaction never resolves on ganache with Websocket and no confirmation or receipt event triggered beta49-52 Increasing of the block number for the confirmation workflow over HTTP is wrong. Apr 8, 2019
@szerintedmi
Copy link
Author

szerintedmi commented Apr 10, 2019

@nivida :
I'm glad that @eduardonunesp found the cause of the issue. I'm just slightly confused that you changed the issue title to http. Our primary concern is websocket - will the fix of this will resolve the ganache confirmation issue on websocket too ?

Also, we identified an other issue / inconsistency in this thread, wondering about that too:

A tx never resolves with await or .then() if there is an .on("receipt") attached. If I remove the .on("receipt") then it resolves.
If it's an intended behaviour then a mention in docs might be helpful for others.

@nivida
Copy link
Contributor

nivida commented Apr 10, 2019

As explained was it not thought to mixup the promise and eventemitter and we had issues with unresolved promise rejections. I will add a sentence to the docs :)

I‘ve tested it with geth and parity over an websocket connection but will definitely test it with the ganache „node“ too.

This will get fixed on Monday after my short vacation.

Btw.: You configured the transaction module options right?

@szerintedmi
Copy link
Author

I‘ve tested it with geth and parity over an websocket connection but will definitely test it with the ganache „node“ too.

thanks @nivida ! Ganache is a crucial part of our workflow for automated testing all our dependent modules. I think it's true for most bigger projects. So breaking ganache blocks us from upgrading web3, we are currently stuck at beta33 for long now.
As I mentioned I more than happy to contribute with some basic integration tests if you agree that it would beneficiary to add it to your CI pipeline.

This will get fixed on Monday after my short vacation.

enjoy it and don't check github alerts :)

Btw.: You configured the transaction module options right?

You mean transactionConfirmationBlocks ? I followed documentation:

{
  defaultBlock: "latest",
  transactionConfirmationBlocks: 3,
  transactionBlockTimeout: 5
}

@princesinha19
Copy link
Contributor

@szerintedmi can you please change transactionConfirmationBlocks to 1, As you are using testrpc.

{
   transactionConfirmationBlocks: 1
}

@szerintedmi
Copy link
Author

szerintedmi commented Apr 11, 2019

@szerintedmi can you please change transactionConfirmationBlocks to 1, As you are using testrpc.

{
   transactionConfirmationBlocks: 1
}

@princesinha19 : trust me I did play with a lot of different settings. Please read the whole thread and see my code example & gist where it's 1 :)
Anyhow I think it should work with transactionConfirmationBlocks: 3 too (just as it used to).

@princesinha19
Copy link
Contributor

Ya, I tested with a web3socket connection too. It's not working we will try to resolve it.

@iantanwx
Copy link

Can confirm that I am facing the same issue. Happy to help with PR/tests if necessary, as I am blocked on a fairly mission-critical project.

@d10r
Copy link

d10r commented Jun 4, 2019

I'm using web3 1.0.0-beta.55 with the latest version of ganache-cli. Using await for transaction sending just blocks forever.
This should work with default settings - like it used to.

@einaralex
Copy link

@d10r do you have an event listener for 'reciept' ?
.on("receipt", receipt => { ... })

When I removed that and get the reciept from the confirmation event, it doesn't hang anymore.
.on('confirmation', (confNumber, receipt) => { ... })

@levino
Copy link
Contributor

levino commented Jul 5, 2019

I'm using web3 1.0.0-beta.55 with the latest version of ganache-cli. Using await for transaction sending just blocks forever.
This should work with default settings - like it used to.

Same here.

@d10r do you have an event listener for 'reciept' ?
.on("receipt", receipt => { ... })

When I removed that and get the reciept from the confirmation event, it doesn't hang anymore.
.on('confirmation', (confNumber, receipt) => { ... })

This is just WTF???

@d10r
Copy link

d10r commented Jul 12, 2019

@einaralex no I don't. Just await web3.eth.sendTransaction(.. in a test.

@boyuanx
Copy link

boyuanx commented Jul 21, 2019

For me, neither await nor .then works. I had to resort to using .on('confirmation', (confNumber, receipt) => { ... }).

Using the latest version of web3.js and Ganache.

@7-of-9
Copy link

7-of-9 commented Apr 12, 2020

I'm seeing failure of promise-wrapped sendSignedTransaction to fire "confirmation" event (and "receipt") on a websocket ganache connection. Works fine on HTTP.

Removing "receipt" handler makes no difference.

"web3": "^2.0.0-alpha"
transactionConfirmationBlocks = 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Addressing a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.