Reimplement Dandelion Transactions #80
Conversation
complete inv/hash rewrite with dandelion queues Signed-off-by: barrystyle <[email protected]>
|
Awesome work! Thanks for doing this. Will get it reviewed. |
|
ACK. Thanks again Barry for the work on this. Compiling works fine & client has run for many hours smoothly without any errors I can see. Would be great to get this merged into 8.22 and then merged into develop. |
There was a problem hiding this comment.
cACK
Lots of great work @barrystyle. Appreciate your efforts!
There was a problem hiding this comment.
ACK
Fantastic work on this @barrystyle . This was a heavy lift and we really appreciate your hard work on this. I think we need to attack the failing GitHub actions before we merge 8.22 into develop, but I'll add my comments to that effect on the feature branch.
|
Thanks for all you do Barry! Much appreciated |
|
This PR looks good to me. I am having a hard time to understand why the wallet tests fail. Even with Dandelion disabled, the wallet transaction tracker now seems to process some mempool transactions twice, which results in an invalid check. Currently, we have two ways to proceed: I am preferring the latter, because I am also working on the functional tests at the moment which will be validating Dandelion anyway. This will also help us to stay up to date with Bitcoin upstream changes. |
This. I thought it mightve been a preexisting failed test but I dont think thats the case. |
|
Had the same idea regarding that convenience function. Great job! 💪 |
SmartArray
changed the title
| @@ -79,15 +79,25 @@ static const bool DEFAULT_BLOCKSONLY = false; | |||
| static const int64_t DEFAULT_PEER_CONNECT_TIMEOUT = 60; | |||
| /** Number of file descriptors required for message capture **/ | |||
| static const int NUM_FDS_MESSAGE_CAPTURE = 1; | |||
| /** The default setting for dandelion transactions */ | |||
| static const bool DEFAULT_DANDELION = true; | |||
There was a problem hiding this comment.
One last thing before I ACK this PR:
We should consider having Dandelion disabled by default.
Otherwise we will lose DigiByte's instant transactions because all transactions will have to enter the stempool first. People can still enable the protocol by passing -dandelion 1 when launching their own nodes.
The iOS wallet supports this too by broadcasting protocol internal dandelion_tx transactions.
I know this was enabled before, but looking at it in the past, I thought that normal transactions entered the mempool and only 'special' transactions would enter the stempool.
@JaredTate @gto90 @barrystyle @ycagel
Would love to hear your thoughts about this.
By the way, this will fix the CI tests too.
Alternatively we can add gArgs.ForceSetArg("-dandelion", "0"); to the wallet_tests
There was a problem hiding this comment.
Forget what I just said. It's fine when it's enabled by default.
@barrystyle Can you add another commit in wallet_tests.cpp at line 700?
gArgs.ForceSetArg("-dandelion", "0");
There was a problem hiding this comment.
I think it is beneficial to leave Dandelion enabled by default, to grant new users extra privacy by default. Perhaps it could be added as a popup during the install process? Giving the user direct control. Not sure what all that would entail as it would take GUI work too. From my experience even with dandelion txs are still quick enough to seem fast.
SmartArray
force-pushed
the
feature/dandelion
branch
2 times, most recently
from
8cc38c1 to
2b25c65
Compare
…ot enter stempool and effects can be seen immediately
|
Sorry for the force push. I thought I did that to a local branch, but it turned out to be the remote branch. I managed to add the missing commit in order to have this branch merged. |
|
After some investigation it is clear that every test case succeeded. The benchmark tests on Mac caused a segfault. I don't think this is critical for this PR. Merging this PR. |
PR contains bug tested & pool trialed implementation of dandelion, with appropriate modifications to tests.