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

Internal Chain Fork Detector #1925

Open
zookozcash opened this Issue Dec 5, 2016 · 6 comments

Comments

@zookozcash

zookozcash commented Dec 5, 2016

This inspects the network to detect a chain fork in which different nodes are following different forks for more than a few [*] blocks. If it detects such a chain fork, it alerts the user.

Ideally, I think, it should rely on the Block Observatory and should be inspecting the "catalog of all blocks" produced by that.

[*] 6, because #952

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Dec 6, 2016

Contributor

Status update: I decided against using the DNS seeder as a starting point, because it didn't implement any block-parsing logic at all, and therefore could not collect the types of information necessary to detect chain forks. Instead, I have forked https://github.com/jgarzik/pynode which implements a basic Bitcoin node and collects blocks from a chosen peer.

I have updated it to work with the latest python-bitcoinlib, which I am porting to Zcash. All that remains is to fix the bugs in my JoinSplit parser, and then we can use pynode as-is to collect blocks from a single peer. Thus the first trivial version of a general chain fork detector would be to run several pynode instances, and then manually compare their blocks.

Tomorrow, I will finish chasing down the parsing bugs, and work on hacking multi-peer connectivity into pynode.

Contributor

str4d commented Dec 6, 2016

Status update: I decided against using the DNS seeder as a starting point, because it didn't implement any block-parsing logic at all, and therefore could not collect the types of information necessary to detect chain forks. Instead, I have forked https://github.com/jgarzik/pynode which implements a basic Bitcoin node and collects blocks from a chosen peer.

I have updated it to work with the latest python-bitcoinlib, which I am porting to Zcash. All that remains is to fix the bugs in my JoinSplit parser, and then we can use pynode as-is to collect blocks from a single peer. Thus the first trivial version of a general chain fork detector would be to run several pynode instances, and then manually compare their blocks.

Tomorrow, I will finish chasing down the parsing bugs, and work on hacking multi-peer connectivity into pynode.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Dec 9, 2016

Contributor

Parsing bugs have been resolved, and the only remaining issue is that transaction signature validation isn't working. But that isn't necessary for a chain fork detector, so I've worked around it for now.

Contributor

str4d commented Dec 9, 2016

Parsing bugs have been resolved, and the only remaining issue is that transaction signature validation isn't working. But that isn't necessary for a chain fork detector, so I've worked around it for now.

@nathan-at-least nathan-at-least changed the title from General Chain Fork Detector to General Custom Chain Fork Detector Jan 9, 2017

@nathan-at-least nathan-at-least added this to the 1.0.5 milestone Jan 9, 2017

@bitcartel bitcartel modified the milestones: 1.0.6, 1.0.5 Jan 16, 2017

@bitcartel bitcartel changed the title from General Custom Chain Fork Detector to General Chain Fork Detector Jan 16, 2017

@bitcartel bitcartel modified the milestones: 1.0.7, 1.0.6 Feb 6, 2017

@str4d str4d modified the milestones: 1.0.7, 1.0.8 Mar 10, 2017

@nathan-at-least nathan-at-least modified the milestones: 1.0.9, 1.0.8 Mar 20, 2017

@nathan-at-least nathan-at-least removed this from Work Queue in Continuous Improvement May 10, 2017

@nathan-at-least nathan-at-least removed this from the 1.0.9 milestone May 10, 2017

@nathan-at-least

This comment has been minimized.

Show comment
Hide comment
@nathan-at-least

nathan-at-least May 10, 2017

Contributor

Bumped from 1.0.9, mainly because I don't know how to evaluate if I can close this ticket, and because the work is decoupled from our release, and we're over-capacity for 1.0.9.

Contributor

nathan-at-least commented May 10, 2017

Bumped from 1.0.9, mainly because I don't know how to evaluate if I can close this ticket, and because the work is decoupled from our release, and we're over-capacity for 1.0.9.

@nathan-at-least nathan-at-least moved this from In Progress to Nominated for Release in Monitoring, Metrics, and Analysis Jul 3, 2017

@nathan-at-least

This comment has been minimized.

Show comment
Hide comment
@nathan-at-least

nathan-at-least Jul 13, 2017

Contributor

I propose closing this ticket with a worse-is-better solution (thus unblocking it from the Block Observatory):

  • we set up multiple detector nodes (Bonus: running a variety of release versions, #2483).
  • we have a monitoring process collect the getchaintips output from all of these nodes.
  • that monitoring process issues alerts whenever a branchlen >= K is observed. Let's say K=6 for now. (Because getchaintips is persistent, we need to be able to explicitly silence alerts for already observed branches.)
  • that monitoring process should feed all results into a metrics system. (Edit: Again, because getchaintips results are persistent, we need to ensure metrics are idempotent / correctly deduplicated.)

I believe we already have all 4 of these implemented. If so we could close the ticket. I'd like a review of all four pieces, first, though. In particular, I believe the alerting system and metrics aggregation are buggy and need improvement (which should be specified in separate tickets).

Contributor

nathan-at-least commented Jul 13, 2017

I propose closing this ticket with a worse-is-better solution (thus unblocking it from the Block Observatory):

  • we set up multiple detector nodes (Bonus: running a variety of release versions, #2483).
  • we have a monitoring process collect the getchaintips output from all of these nodes.
  • that monitoring process issues alerts whenever a branchlen >= K is observed. Let's say K=6 for now. (Because getchaintips is persistent, we need to be able to explicitly silence alerts for already observed branches.)
  • that monitoring process should feed all results into a metrics system. (Edit: Again, because getchaintips results are persistent, we need to ensure metrics are idempotent / correctly deduplicated.)

I believe we already have all 4 of these implemented. If so we could close the ticket. I'd like a review of all four pieces, first, though. In particular, I believe the alerting system and metrics aggregation are buggy and need improvement (which should be specified in separate tickets).

@nathan-at-least nathan-at-least changed the title from General Chain Fork Detector to Internal Chain Fork Detector Jul 14, 2017

@nathan-at-least

This comment has been minimized.

Show comment
Hide comment
@nathan-at-least

nathan-at-least Jul 14, 2017

Contributor

Ah, I just realized when @zookozcash says "the user" in the description I thought "ZcashCo operations team" but maybe he meant "any user of Zcash". That totally changes things. I propose we bite off the first goal (internal alerting system) as the first milestone, then followup with a public-facing interface (and cloneable open code others can deploy).

So I just renamed this, and will file a new ticket for the public facing service: #2536

Contributor

nathan-at-least commented Jul 14, 2017

Ah, I just realized when @zookozcash says "the user" in the description I thought "ZcashCo operations team" but maybe he meant "any user of Zcash". That totally changes things. I propose we bite off the first goal (internal alerting system) as the first milestone, then followup with a public-facing interface (and cloneable open code others can deploy).

So I just renamed this, and will file a new ticket for the public facing service: #2536

@nathan-at-least

This comment has been minimized.

Show comment
Hide comment
@nathan-at-least

nathan-at-least Jul 14, 2017

Contributor

@str4d I don't quite follow your approach. I imagined polling the result of getchaintips from multiple nodes. How does your approach compare to that? (It sounds more involved to me.)

Contributor

nathan-at-least commented Jul 14, 2017

@str4d I don't quite follow your approach. I imagined polling the result of getchaintips from multiple nodes. How does your approach compare to that? (It sounds more involved to me.)

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