Implement spvnode.sendTX() and spvnode.relay() #417

OrfeasLitos · 2018-02-17T20:57:18Z

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.

lib/node/spvnode.js

OrfeasLitos · 2018-02-18T21:54:08Z

All done

tuxcanfly

Tested OK.

Earlier a transaction relayed by us was only added to the filter if a peer relayed it back.

Sidenote: this cannot be easily tested without being able to configure wallet port (see #418)

bucko13 · 2018-02-19T17:25:07Z

What's the reasoning for no longer returning promises?

lib/node/spvnode.js

OrfeasLitos · 2018-02-19T17:39:27Z

I did not return following the example of the full node:

bcoin/lib/node/fullnode.js

Line 294 in 5382304

async sendTX(tx) {

and

bcoin/lib/node/fullnode.js

Line 329 in 5382304

async relay(tx) {

.
Should I add the returns? Should the full node be changed as well?

nodech · 2018-02-19T17:45:05Z

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

nodech · 2018-02-19T17:45:33Z

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

OrfeasLitos · 2018-02-19T17:51:01Z

Done

nodech · 2018-02-20T08:25:06Z

lib/node/spvnode.js

  }

  /**
   * Broadcast a transaction. Silence errors.
   * @param {TX} tx
-   * @returns {Promise}


you still need this returns in both methods.

I can do it, but won't this make the spv node have a different API from the full node? Is there a reason for this difference?

fullnode lacks this line in docs. Every async method returns Promise.

OrfeasLitos · 2018-02-20T13:30:34Z

Now it should be ok?

This note contained some provisional bug fixes made to bcoin/lib/node/spvnode.js, along with the suggestions by the bcoin devs on how to correctly fix those bugs. Since PR bcoin-org/bcoin#417 has been published, this note is redundant.

braydonf · 2018-06-28T01:24:10Z

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.

OrfeasLitos · 2018-06-28T12:01:23Z

@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?

braydonf · 2018-06-28T18:03:27Z

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

OrfeasLitos · 2018-06-28T18:07:42Z

You're referring to this:

bcoin/lib/node/fullnode.js

Line 284 in f00ed98

await this.pool.broadcast(item);

right?

braydonf · 2018-06-28T18:08:36Z

Yep, and the same exists in spvnode.js

OrfeasLitos · 2019-02-18T09:30:36Z

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

tynes · 2019-02-18T19:20:10Z

lib/node/spvnode.js

   * @param {TX} tx
   * @returns {Promise}
   */

-  sendTX(tx) {
-    return this.broadcast(tx);
+  async sendTX(tx) {


To protect users from getting banned by sending an invalid tx, would it make sense to first check tx.verify() before anything else?

Fixed following the full node example:

bcoin/lib/mempool/mempool.js

Lines 785 to 788 in f585d86

const [valid, reason, score] = tx.checkSanity();

if (!valid)

throw new VerifyError(tx, 'invalid', reason, score);

bcoin/lib/node/fullnode.js

Lines 297 to 303 in f585d86

} catch (err) {

if (err.type === 'VerifyError' && err.score === 0) {

this.error(err);

this.logger.warning('Verification failed for tx: %h.', tx.hash());

this.logger.warning('Attempting to broadcast anyway...');

this.broadcast(tx);

return;

codecov-io · 2019-02-18T22:14:16Z

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%)`	⬇️
lib/node/spvnode.js	`54.9% <53.84%> (+21.93%)`	⬆️
lib/workers/worker.js	`0% <0%> (-100%)`	⬇️
lib/hd/words/chinese-traditional.js	`0% <0%> (-100%)`	⬇️
lib/hd/words/french.js	`0% <0%> (-100%)`	⬇️
lib/hd/words/japanese.js	`0% <0%> (-100%)`	⬇️
lib/hd/words/chinese-simplified.js	`0% <0%> (-100%)`	⬇️
lib/hd/words/italian.js	`0% <0%> (-100%)`	⬇️
lib/script/scripterror.js	`6.66% <0%> (-93.34%)`	⬇️
lib/blockstore/file.js	`5.45% <0%> (-90.46%)`	⬇️
... 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.

OrfeasLitos · 2019-02-18T22:16:16Z

Also rebased and added tests for TX emission

OrfeasLitos · 2019-04-04T10:05:44Z

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

pinheadmz · 2019-11-07T20:25:53Z

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:

Launch full node:

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

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

Get SPV wallet address and generate coins to it from full node.
Make and sign but do not broadcast a transaction from the SPV node:

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

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

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

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

test/spvnode-test.js

pinheadmz

Thanks for your work on this, @OrfeasLitos and your patience in review!

tuxcanfly

Tested OK. Fixed minor nits.

Resolved.

tuxcanfly · 2019-11-16T05:29:13Z

@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.

tuxcanfly · 2019-11-16T05:31:16Z

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.

test/spvnode-test.js

Co-authored-by: Christopher Jeffrey <chjjeffrey@gmail.com>

The definitions of these two functions were not implemented previously, they both just called this.broadcast(). Now sendTX() broadcasts only if it wouldn't hurt its score and emits the tx only if it is watched; relay() calls sendTX() and silences errors, just like for the full node.

OrfeasLitos force-pushed the spv-send-relay branch 2 times, most recently from 900da26 to a54dd34 Compare February 17, 2018 22:11

OrfeasLitos changed the base branch from master to wallet-rewrite February 17, 2018 22:11

OrfeasLitos force-pushed the spv-send-relay branch 2 times, most recently from b3d9585 to c63e666 Compare February 17, 2018 22:24

nodech previously requested changes Feb 17, 2018

View reviewed changes

lib/node/spvnode.js Outdated Show resolved Hide resolved

lib/node/spvnode.js Outdated Show resolved Hide resolved

lib/node/spvnode.js Outdated Show resolved Hide resolved

OrfeasLitos force-pushed the spv-send-relay branch from c63e666 to 50badc1 Compare February 18, 2018 21:53

tuxcanfly approved these changes Feb 19, 2018

View reviewed changes

nodech reviewed Feb 19, 2018

View reviewed changes

lib/node/spvnode.js Outdated Show resolved Hide resolved

OrfeasLitos force-pushed the spv-send-relay branch from 50badc1 to 6640b15 Compare February 19, 2018 17:50

nodech reviewed Feb 20, 2018

View reviewed changes

OrfeasLitos force-pushed the spv-send-relay branch from 6640b15 to 2875f06 Compare February 20, 2018 13:29

OrfeasLitos force-pushed the spv-send-relay branch 2 times, most recently from 4fd1cd2 to e426e43 Compare February 21, 2018 11:03

tynes reviewed Feb 18, 2019

View reviewed changes

OrfeasLitos force-pushed the spv-send-relay branch from e426e43 to ab5f9cb Compare February 18, 2019 21:54

OrfeasLitos force-pushed the spv-send-relay branch 2 times, most recently from 75a5e7c to 1f65e4a Compare February 18, 2019 22:14

OrfeasLitos changed the base branch from wallet-rewrite to master February 18, 2019 22:15

This was referenced Apr 3, 2019

Emit TX when sent by SPV node #697

Closed

Automatically set walletdb.spv according to client #621

Open

pinheadmz mentioned this pull request Nov 5, 2019

In SPV mode, broadcast transactions are not always added to wallet. #631

Open

pinheadmz reviewed Nov 7, 2019

View reviewed changes

test/spvnode-test.js Outdated Show resolved Hide resolved

pinheadmz reviewed Nov 7, 2019

View reviewed changes

test/spvnode-test.js Outdated Show resolved Hide resolved

OrfeasLitos force-pushed the spv-send-relay branch 2 times, most recently from a94d6d1 to 161d76d Compare November 8, 2019 10:51

pinheadmz approved these changes Nov 8, 2019

View reviewed changes

pinheadmz added ready for review Ready to be reviewed spv labels Nov 8, 2019

tuxcanfly self-requested a review November 16, 2019 05:19

tuxcanfly approved these changes Nov 16, 2019

View reviewed changes

tuxcanfly requested a review from nodech November 16, 2019 05:21

tuxcanfly approved these changes Nov 16, 2019

View reviewed changes

test/spvnode-test.js Show resolved Hide resolved

test/spvnode-test.js Show resolved Hide resolved

nodech approved these changes Nov 16, 2019

View reviewed changes

tuxcanfly and others added 2 commits November 16, 2019 16:16

pool: catch peer handler errs

d78e46e

Co-authored-by: Christopher Jeffrey <chjjeffrey@gmail.com>

OrfeasLitos force-pushed the spv-send-relay branch from 161d76d to 431cfc4 Compare November 16, 2019 16:19

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

518be85

OrfeasLitos force-pushed the spv-send-relay branch from 431cfc4 to 518be85 Compare November 16, 2019 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement spvnode.sendTX() and spvnode.relay() #417

Implement spvnode.sendTX() and spvnode.relay() #417

OrfeasLitos commented Feb 17, 2018 •

edited by bucko13

Loading

OrfeasLitos commented Feb 18, 2018

tuxcanfly left a comment

bucko13 commented Feb 19, 2018

OrfeasLitos commented Feb 19, 2018

nodech commented Feb 19, 2018

nodech commented Feb 19, 2018

OrfeasLitos commented Feb 19, 2018

nodech Feb 20, 2018

OrfeasLitos Feb 20, 2018

nodech Feb 20, 2018

OrfeasLitos commented Feb 20, 2018

braydonf commented Jun 28, 2018 •

edited

Loading

OrfeasLitos commented Jun 28, 2018

braydonf commented Jun 28, 2018

OrfeasLitos commented Jun 28, 2018 •

edited

Loading

braydonf commented Jun 28, 2018

OrfeasLitos commented Feb 18, 2019

tynes Feb 18, 2019

OrfeasLitos Feb 18, 2019 •

edited

Loading

codecov-io commented Feb 18, 2019 •

edited

Loading

OrfeasLitos commented Feb 18, 2019

OrfeasLitos commented Apr 4, 2019 •

edited

Loading

pinheadmz commented Nov 7, 2019

pinheadmz left a comment

tuxcanfly left a comment

tuxcanfly commented Nov 16, 2019

tuxcanfly commented Nov 16, 2019

	const [valid, reason, score] = tx.checkSanity();

	if (!valid)
	throw new VerifyError(tx, 'invalid', reason, score);

	} catch (err) {
	if (err.type === 'VerifyError' && err.score === 0) {
	this.error(err);
	this.logger.warning('Verification failed for tx: %h.', tx.hash());
	this.logger.warning('Attempting to broadcast anyway...');
	this.broadcast(tx);
	return;

Implement spvnode.sendTX() and spvnode.relay() #417

Are you sure you want to change the base?

Implement spvnode.sendTX() and spvnode.relay() #417

Conversation

OrfeasLitos commented Feb 17, 2018 • edited by bucko13 Loading

OrfeasLitos commented Feb 18, 2018

tuxcanfly left a comment

Choose a reason for hiding this comment

bucko13 commented Feb 19, 2018

OrfeasLitos commented Feb 19, 2018

nodech commented Feb 19, 2018

nodech commented Feb 19, 2018

OrfeasLitos commented Feb 19, 2018

nodech Feb 20, 2018

Choose a reason for hiding this comment

OrfeasLitos Feb 20, 2018

Choose a reason for hiding this comment

nodech Feb 20, 2018

Choose a reason for hiding this comment

OrfeasLitos commented Feb 20, 2018

braydonf commented Jun 28, 2018 • edited Loading

OrfeasLitos commented Jun 28, 2018

braydonf commented Jun 28, 2018

OrfeasLitos commented Jun 28, 2018 • edited Loading

braydonf commented Jun 28, 2018

OrfeasLitos commented Feb 18, 2019

tynes Feb 18, 2019

Choose a reason for hiding this comment

OrfeasLitos Feb 18, 2019 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Feb 18, 2019 • edited Loading

Codecov Report

OrfeasLitos commented Feb 18, 2019

OrfeasLitos commented Apr 4, 2019 • edited Loading

pinheadmz commented Nov 7, 2019

Results:

pinheadmz left a comment

Choose a reason for hiding this comment

tuxcanfly left a comment

Choose a reason for hiding this comment

tuxcanfly commented Nov 16, 2019

tuxcanfly commented Nov 16, 2019

OrfeasLitos commented Feb 17, 2018 •

edited by bucko13

Loading

braydonf commented Jun 28, 2018 •

edited

Loading

OrfeasLitos commented Jun 28, 2018 •

edited

Loading

OrfeasLitos Feb 18, 2019 •

edited

Loading

codecov-io commented Feb 18, 2019 •

edited

Loading

OrfeasLitos commented Apr 4, 2019 •

edited

Loading