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

update Dandelion documentation #2723

Closed

Conversation

@lehnberg
Copy link
Collaborator

commented Apr 2, 2019

Fixes #2719.

@lehnberg lehnberg added the task label Apr 2, 2019
@lehnberg lehnberg added this to the 1.1.0 milestone Apr 2, 2019
@lehnberg lehnberg self-assigned this Apr 2, 2019
@lehnberg lehnberg removed this from the 1.1.0 milestone Apr 2, 2019
@lehnberg lehnberg requested review from quentinlesceller and antiochp Apr 4, 2019
@lehnberg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 4, 2019

@antiochp @quentinlesceller the first draft is now ready for review, let me know what you think, and please notice the questions I've highlighted throughout the description of the Grin implementation, not sure what we do in some of these cases.

@lehnberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

@antiochp @quentinlesceller just another nudge on a review on this (happy to make edits in case you'd want this to look different - just say).

Some specific questions I have about the way this is implemented in Grin:

  1. How does the node choose their outbound relay? A single one at random? At the beginning of each epoch?
  2. Does the node put its own and inbound received transactions into its stempool?
  3. Does the node always forward its own transactions alongside its outbound path, even if it's in fluff mode?
  4. Is the 10s interval of dandelion_monitor configurable?
  5. If a node receives a stem transaction from an inbound node that has already existed in its own stempool that epoch, will it fluff the existing transaction?
@antiochp

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

How does the node choose their outbound relay? A single one at random? At the beginning of each epoch?

Yes. One is randomly chosen from the current set of outbound peers at the beginning of each epoch.

Does the node put its own and inbound received transactions into its stempool?

Yes. Any stem txs received will be added to the local stempool.
Any stem txs received via a wallet push will also be added to local stempool.

Does the node always forward its own transactions alongside its outbound path, even if it's in fluff mode?

No. I believe we diverge from the spec here. There may be room for improvement to make the behavior match the spec. Did it this way as the impl was significantly simpler if we did not need to treat "local" txs differently. Still not entirely clear what the benefit is of always stemming local txs.

Is the 10s interval of dandelion_monitor configurable?

Yes. The following is configurable -

  • epoch_secs
  • embargo_secs
  • aggregation_secs
  • stem_probability

If a node receives a stem transaction from an inbound node that has already existed in its own stempool that epoch, will it fluff the existing transaction?

I believe so yes. This implies something unexpected happened in terms of routing and we immediately fluff in this situation. We probably want to keep an eye on this and see how often this occurs in practice.

Copy link
Member

left a comment

Apologies for not reviewing this earlier. This looks good.
Responded to your specific questions separately.

@antiochp

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

"Always stem local" behavior implemented here - #2876

Turns out the main thing preventing us from doing this was how hard it makes Dandelion testing if we cannot get "local" txs to participate in local stem aggregation.

So made it a configurable option that defaults to following the Dandelion++ paper.


The following config will set your local node to aggregate all txs it sees and fluff them periodically -

aggregation_secs = 30
stem_probability = 0
always_stem_local = false
@lehnberg lehnberg force-pushed the lehnberg:dandelion_docs branch from 79c1143 to d83bcc8 Jun 6, 2019
@lehnberg lehnberg marked this pull request as ready for review Jun 6, 2019
@lehnberg lehnberg changed the title [WIP] update Dandelion documentation update Dandelion documentation Jun 6, 2019
@lehnberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 6, 2019

@antiochp thanks for the review, the comments, and the improvement! I've updated the docs to reflect your answers I think and this is now ready to be merged.

Just a quick note regarding this one:

Is the 10s interval of dandelion_monitor configurable?

Yes. The following is configurable -

epoch_secs
embargo_secs
aggregation_secs
stem_probability

Actually, I don't think the parameters you list here relate to dandelion_monitor no? I'm referring specifically to what you write in the description of #2628:

Dandelion monitor runs every 10s

Is this every 10s configurable? And if so, what's the parameter?

@antiochp

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Oh I see what you're asking - I misunderstood before.

The 10s is not configurable no, its simply an implementation detail.
Every 10s we check the various (configurable) timers, 30s for aggregation etc.

@lehnberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 6, 2019

Oki cool - so then this documentation is accurate and is ready for merging unless there is other feedback. ✌️

@antiochp antiochp added this to the 2.x.x milestone Jun 7, 2019
@sesam

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

On which branch should this be merged?

And, reading this

stem_probability = 0
always_stem_local = false

I would have guessed such settings makes stemming not happen.

@lehnberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 3, 2019

On which branch should this be merged?

Good question, if I understand correctly our current docs are now outdated. Any reason why this shouldn't be merged into master @antiochp?

@antiochp

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

We're holding off touching master until after the HF I believe.
But feel free to merge it into the milestone/2.x.x branch for now.

@lehnberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

@antiochp cool - does that mean that I need to close and reopen the PR?

@jaspervdm

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

No, you can change the branch you want to merge in on the top of the page, when you press the "Edit" button

@lehnberg lehnberg changed the base branch from master to milestone/2.x.x Jul 8, 2019
yeastplume and others added 5 commits Jun 5, 2019
* generate txhashset archives on 250 block intervals.

* moved txhashset_archive_interval to global and added a simple test.

* cleaning up the tests and adding license.

* increasing cleanup duration to 24 hours to prevent premature deletion of the current txhashset archive

* bug fixes and changing request_state to request height using archive_interval.

* removing stopstate from chain_test_helper to fix compile issue
@lehnberg lehnberg force-pushed the lehnberg:dandelion_docs branch from d83bcc8 to 1d22b3e Jul 8, 2019
@lehnberg

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

Thanks, I've re-opened a cleaned up version as #2942 to be merged against milestone/2.x.x rather than this one. Closing.

@lehnberg lehnberg closed this Jul 8, 2019
@lehnberg lehnberg deleted the lehnberg:dandelion_docs branch Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.