Question, Why the errors silenced/swallowed when an mtx tried to be sent using `Wallet.sendMTX()` ?

[rozaydin]

This is a query to understand the underlying reasoning, whenever a tx created using Wallet.sendMTX() there is a potential case a tx returned but due to an error tx never makes into mempool, error logged and swallowed.

lib/wallet/nodeclient.js:

 /**
   * Send a transaction. Do not wait for promise.
   * @param {TX} tx
   * @returns {Promise}
   */

  async send(tx) {
    this.node.relay(tx);
  }

lib/node/fullnode.js:

  /**
   * Add transaction to mempool, broadcast. Silence errors.
   * @param {TX} tx
   * @returns {Promise}
   */

  async relay(tx) {
    try {
      await this.sendTX(tx);
    } catch (e) {
      this.error(e);
    }
  }

More specifically, what would be the drawback of changing below 2 files like in the following PR: Sample PR (ps: just for showing the changes)

and allow errors (when occurred) to propagate, thus prevent http clients thinking they have a tx, but instead the tx failed and will never get mined ?

[pinheadmz]

I couldn't agree more with this and in fact the issue is already tracked here:

"Attempting to broadcast anyway" must die #552

It should be a very simple PR (just remove the try/catch) but I'd like to see it accompanied by tests with both success and failure cases. In addition, it would be super useful to also have an rpc testmempoolaccept like the one in bitcoind: https://chainquery.com/bitcoin-cli/testmempoolaccept

The "broadcast anyway" logic is a relic from bcoin, where it was added for some weird Purse.io use case -- I think they might have had issues with parallel wallets broadcasting TXs out of order or some fee issues or something...

~So, I am going to close this issue as a duplicate of #552 Your Sample PR is cool, but again I think it can be even simpler if you look at the "broadcast anyway" snippet.~

[pinheadmz]

@rozaydin what kind of errors are you looking for? Some errors for example require context that only the full node knows about (double spend, minimum relay fee) and there's only a few things the wallet does know about (tx not signed correctly).

[nodech]

I am not 100% about this, but I will try to cover the context of that path.

wallet <-> node interface is not as straightforward as nodeclient -> fullnode. Actually this interface is used for several setups, namely:

• fullnode + wallet running together (your case), these two files come into play: lib/node/fullnode and lib/wallet/nodeclient
• spvnode and wallet running together, we have lib/node/spvnode <-> lib/wallet/nodeclient
• fullnode and wallet running separately will result using HTTP instead of nodeclient, so we will have lib/wallet/client (Wrapper around hs-clients NodeClient), so lib/node/fullnode and lib/wallet/client
• spvnode and wallet running separately, HTTP with lib/wallet/client (hs-client wrapper) and lib/node/spvnode.

There are many small and big differences between fullnode and spvnode, their handling and awareness of the errors. And there are other issues that come into play when we use HTTP (timeout, http conn error, etc.).

SPV can't answer most of the questions that fullnode can and as mentioned above HTTP may not be able to deliver the errors consistently. For example, fullnode can check against local mempool policies while SPV will broadcast it. Also note, Wallet on HTTP maybe not be aware of the type(spv, full) of the node, even if it was current wallet is not "paired" with single type of node(spv, full, pruned) and it can be replaced (e.g. go from full to spv). This issue probably needs addressing to reduce complexity

This will result pretty inconsistency API in the end and basically break down when you "simply" switch to http or just use spv. Meaning code will need additional awareness of the "Node" and the transport in use, and then why have this abstraction at all.

Unfortunately, I don't remember intricacies of all things and will need a good look to really give you answer desired, but thought will still write down what's happening around the APIs.

[rozaydin]

@pinheadmz We want to ensure when interacting with the wallet through the api (specifically the http one), if it succeeds get the tx with 200 ok, if it fails, fail it with 500. At the moment we do not need to discriminate the type of errors at all, but that would be nice to have.

[rozaydin]

@nodar-chkuaselidze Those are cases i didnt consider at all, thank you for documenting them!

[RevCBH]

@nodar-chkuaselidze, re: "Meaning code will need additional awareness of the "Node" and the transport in use, and then why have this abstraction at all."

In our (Namebase's) particular use case that's an instructive question. Maintaining that abstraction is likely a good thing for the broader ecosystem, and we wouldn't mind maintaining a separate, more specialized, wallet interface. Ideally this would be via a plugin so we could run vanilla hsd.

Might it be worth thinking about how we could update the node/wallet to maintain existing interface behavior while enabling these reliability semantics for interfaces that explicitly don't support SPV?

[rozaydin]

@pinheadmz @nodar-chkuaselidze I did some extensive testing, seems like it's working ok (sort of quite good, or i am doing sth very very wrong). One thing i would like to ask your opinion is within wallet.js, i have changed the order of below two lines: (Present in the sample, PR)

...
    await this.wdb.send(tx, sendBlocking);

    await this.wdb.addTX(tx);

    return tx;

Hoping to prevent wallet state change if the mempool rejects the tx. Do you happen to know any side effects/cases of doing it like above ?

[nodech]

@rozaydin I will need to have a proper look to give you proper answer. I don't think I have looked into that issue at all. But it can definitely be improvement for the hsd wallet. But to me NODE HTTP API improvements are more crucial, especially for wallet (and potentially tool) developers.

P.S. node.relay and etc are also is in sockets, that may need to be addressed as well? not sure.

@RevCBH I believe there is a room for improvements for HSD fullnode clients. As I have mentioned above, we definitely can improve Node HTTP API for wallet/tool developers, for more control and errors.

I will list example of some endpoints, but is probably good idea to go through HTTP Endpoints and list all shortcomings. There is lack of tests around Node HTTP and is probably good place to start from.

Example for improvements could be to have diverse endpoints (or options) for sending TX/Claims:

• Send to our mempool/chain and broadcast only then, return error (Check local mempool policies)
• Ignore local mempool, only chain checks and broadcast anyway, only chain errors (no mempool errors)
• Don't check chain/mempool and broadcast, no errors (This is pretty dangerous thing to do, considering you can get banned from the network so maybe we should not support this ? or have some obscure options haha)

This might need some form of compatibility with SPV Node, but I believe not getting chain/mempool errors when using SPVNode is expected and should be OK.

PR #552 is also on track with this issue, I believe. @pinheadmz