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

Dandelion support #588

Merged
merged 23 commits into from
Feb 16, 2020
Merged

Dandelion support #588

merged 23 commits into from
Feb 16, 2020

Conversation

aguycalled
Copy link
Member

@aguycalled aguycalled commented Aug 19, 2019

This PR adds support for Dandelion as described in https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki

Includes unit test.

@mxaddict
Copy link
Contributor

mxaddict commented Sep 8, 2019

utACK

@mxaddict
Copy link
Contributor

I don't fully understand the implementation, but changes seem to make sense, still reading through the unit test

@chasingkirkjufell
Copy link
Contributor

set up 6 nodes with config file adding one neighboring node 1 > 2 > 3 > 4 > 5 > 6 > 1 so each node has 2 connections, one added one found.

tested sending transactions from 1 to 4 and 1 to 5, all worked with a delay time around 20 sec to over a minute. monitoring the mempoolinfo, the first neighboring node (2) usually got the transaction around 15-30 sec after it's created, and then each node adding a delay time between 0 - 15 seconds was observed.

One thing worth mentioning is that when 1 creates a transaction, the transaction always goes to 6, and not 2. same behavior also happened when 2 creates a transaction to 6, the broadcasting always go to 1 only and not 2(added node in config file of 2).

@chasingkirkjufell
Copy link
Contributor

could be also related to the order of the way i launch my nodes. i launched in sequence of 1 6 5 4 3 2.

@chasingkirkjufell
Copy link
Contributor

chasingkirkjufell commented Sep 27, 2019

from testing it seems that once a node boots up, it chooses one of the connected node to broadcast dandelion transactions, and it'll always be that node for future transactions. might be better if the node selection is randomized every time from connected nodes. (could also be result of added node syndrome described below)

once the selected node shuts down, dandelion transactions became really slow to use other nodes or stuck in mempool completely. rebooting the sending client will reset the outgoing node selection and flush the stuck transaction (stuck transaction could be the result of the observation below where added node is not preferred by dandelion since my setup was 1>2>3>4>5>6>1, so disliking 2 from one, 1 broadcasts transaction normally this way 1>6>5.... and if 6 is shut down, going through 2 suffer the added node syndrome described below)

tested sending up to 10 transactions at a time. it'll all go through the same node but at a different delayed time. good stuff.

(below is referred the added node syndrome)
dandelion protocol doesn't seem to like "addnode=xxx.xxx.xxx". if only connected to a node which was specified in addednode in config file, the dandelion transaction became extremely slow ( > 1 min to broadcast to that added node or completely hangs) , it could take so long that the transaction basically lingers in the network and won't show up until 10 - 30 minute later in devnet (if all nodes down the line all share the same setup, e.g. the nearest node is specified in addnode=xxx.xxx.xxx). it could have something to do with added node not having ports attached to the address.

@aguycalled
Copy link
Member Author

Paid 600NAV to NL9jPW75P4kNMdvQmFBpYVkZ3sABQtXfYY
600 (major ux issue)
eb61fc74b998ea01a8ef5d2a68f948f699dca453e4b3661577d18c2f9cea8719

@chasingkirkjufell
Copy link
Contributor

working beautifully now. great job.

@navbuilder
Copy link

A new build of 336502a has completed succesfully!
Binaries available at https://build.nav.community/binaries/dandelion

@navbuilder
Copy link

A new build of 61a672b has completed succesfully!
Binaries available at https://build.nav.community/binaries/dandelion

@0x2830
Copy link
Contributor

0x2830 commented Feb 14, 2020

Needs to be rebased.

@navbuilder
Copy link

A new build of 5c25e15 has completed succesfully!
Binaries available at https://build.nav.community/binaries/dandelion

@navbuilder
Copy link

A new build of b8109d1 has completed succesfully!
Binaries available at https://build.nav.community/binaries/dandelion

Copy link
Contributor

@mxaddict mxaddict left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newly added siphash test makes sense

I tested with 4 local nodes in circular connection, broadcasting seems to work fine

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

Successfully merging this pull request may close these issues.

None yet

5 participants