The definitions of these two functions were not implemented previously, they both just called this.broadcast(). Now sendTX() adds the tx to the txFilter of the pool and emits the tx; relay() calls sendTX() and silences errors, just like for the full node.

All done

What's the reasoning for no longer returning promises?

I did not return following the example of the full node: https://github.com/bcoin-org/bcoin/blob/5382304ac4bb52d6c317530e18075a48514998e0/lib/node/fullnode.js#L294 and https://github.com/bcoin-org/bcoin/blob/5382304ac4bb52d6c317530e18075a48514998e0/lib/node/fullnode.js#L329 . Should I add the returns? Should the full node be changed as well?

Nope, I thought It would break the API. then you need to add async keyword to sendTX and await this.broadcast

When it returned Promise, it would work with other async/await. This will just break.

Done

Now it should be ok?

The fullnode sendTx method doesn't await for this.broadcast(tx) and only when trying to the mempool. Was that the intention? Following it through and an await for broadcast() would resolve as soon as one of the peers requests the data for the transaction from the inventory announcement message.

@braydonf maybe this comment is more relevant in #420, since the fullnode sendTX() is most recently changed there and is not changed here.

This is intentional. When adding to the mempool, we need to see whether the tx is an orphan, so we have to await for addTX() to return. Note that this procedure does not need any network interaction, thus the resolution should happen quickly.

As you said, the resolution of await this.broadcast(tx) depends on network factors, such as whether any peer is connected and whether any peer sends a getdata for this particular tx. In case no peer is connected, sendTX() may complete in an indefinite amount of time. We wouldn't want to wait for too long and anyway from a semantic point of view, as long as sendTX() adds the broadcast job to the array of jobs, its mission is complete, why wait even more?

Right, so there is an await this.pool.broadcast with the error being emitted from the node from there.

You're referring to this: https://github.com/bcoin-org/bcoin/blob/f00ed98eead4583e6b1020dbff9c4bf2d3a527de/lib/node/fullnode.js#L284 right?

Yep, and the same exists in spvnode.js

@nodar-chkuaselidze Can we merge either this or #697? Also #681 is ready and we need them for our application.

Codecov Report

Merging #417 into master will decrease coverage by 42.24%. The diff coverage is 41.17%.

@@            Coverage Diff             @@
##           master    #417       +/-   ##
==========================================
- Coverage   61.94%   19.7%   -42.25%     
==========================================
  Files         156     155        -1     
  Lines       26084   26071       -13     
==========================================
- Hits        16157    5136    -11021     
- Misses       9927   20935    +11008

Impacted Files	Coverage Δ
lib/net/pool.js	16.49% <0%> (-14.95%)	:arrow_down:
lib/node/spvnode.js	54.9% <53.84%> (+21.93%)	:arrow_up:
lib/workers/worker.js	0% <0%> (-100%)	:arrow_down:
lib/hd/words/chinese-traditional.js	0% <0%> (-100%)	:arrow_down:
lib/hd/words/french.js	0% <0%> (-100%)	:arrow_down:
lib/hd/words/japanese.js	0% <0%> (-100%)	:arrow_down:
lib/hd/words/chinese-simplified.js	0% <0%> (-100%)	:arrow_down:
lib/hd/words/italian.js	0% <0%> (-100%)	:arrow_down:
lib/script/scripterror.js	6.66% <0%> (-93.34%)	:arrow_down:
lib/blockstore/file.js	5.45% <0%> (-90.46%)	:arrow_down:
... and 101 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2148e6b...518be85. Read the comment docs.

Also rebased and added tests for TX emission

@pinheadmz Can you clarify what changes you propose in this comment?

Ok so since #631 has come up again, I gave this PR another review and I think it is a good solution to a long-standing issue. I rebased on master here: https://github.com/pinheadmz/bcoin/commits/spv-send-relay-2 and fixed a few typos (rename test to SPV Node and replace util/assert with bsert). I was able to confirm this branch solves #631 the following way:

1. Launch full node:

$ bcoin --network=regtest --no-wallet --bip37

2. Launch and connect SPV node with unique ports:

$  bcoin --spv --prefix=~/.bcoin/rtspv --port=11111 --http-port=22222 \
 --wallet-port=33333 --wallet-http-port=44444 --only=127.0.0.1

3. Get SPV wallet address and generate coins to it from full node.

4. Make and sign but do not broadcast a transaction from the SPV node:

$ bwallet-cli --http-port=44444 mktx n1n4wbxax1Q6H1zFmuazJBxvNwv1sVk4d8 0.01

5. In a new terminal, set up a listener for all wallet events from the SPV node:

$ bwallet-cli --http-port=44444 listen

6. Broadcast the transaction hex result from (4) FROM the SPV node:

$ bcoin-cli --http-port=22222 broadcast \
0100000001a7cb2a20b0db5ccfebdcacd8270...

Results:

On master branch, the wallet does not emit any events. On spv-send-relay-2, you catch the usual events TX and Balance

@OrfeasLitos only minor nit. Please use standard commit messages e.g:

spvnode: implement sendTX and relay; emit "tx"

test: add node sendTX test case for "tx" event

Apologies for having this linger for so long.

Also, either squash the commits or move spvnode-test.js to the commit last commit. Because, without the fix, the spv test will fail and break the build at that commit.