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 drand endpoints #4125

Merged
merged 1 commit into from
Oct 14, 2020
Merged

Update drand endpoints #4125

merged 1 commit into from
Oct 14, 2020

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Oct 1, 2020

  • Remove incentinet (should not be used already)
  • Add cloudflare endpoint.
  • Switch PL endpoints to HTTP instead of HTTPS.

@hsanjuan hsanjuan self-assigned this Oct 1, 2020
build/drand.go Outdated Show resolved Hide resolved
@nikkolasg
Copy link
Contributor

My opinion: I would tend to keep HTTPs for three reasons:

  1. biggest one: User is sure that the value comes from the drand node and hasn't been MitM, externally tampered with
  2. Internet in general is tending towards full https everywhere - it's like using IPv6, if we can we should
  3. If the goal is to reduce bandwitdth, we should put most of our efforts into making sure gossipsub is used as much as possible by Lotus node, since that's our primary use case.

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Oct 1, 2020

Sorry, to highlight advantages: HTTPS has more latency. There is also a cost associated with running the endpoints (HTTP requests costs less).

MiTM is not a risk here, as randomness is verified after fetching.

Copy link
Contributor

@willscott willscott left a comment

Choose a reason for hiding this comment

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

i don't think i have a strong preference on the http/https thing.
adding cloudflare is good.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Tests are failing

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Oct 2, 2020

Thanks @willscott

@magik6k
Copy link
Contributor

magik6k commented Oct 4, 2020

Can't sync a fresh node with this PR applied, getting errors like

2020-10-04T17:40:47.513+0200    INFO    chain   chain/sync.go:706       block validation        {"took": 0.303595103, "height": "1", "age": 3519617.513044093}
2020-10-04T17:40:47.513+0200    ERROR   chain   chain/sync.go:1470      failed to validate tipset: validating block bafy2bzacechdx6xd62lcyy7rnyc4uxcxhuwqslcxfvj77fxlwafij3nhzchpy:
    github.com/filecoin-project/lotus/chain.(*Syncer).ValidateTipSet.func1
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/sync.go:620
  - 1 error occurred:
        * failed to validate blocks random beacon values:
    github.com/filecoin-project/lotus/chain.(*Syncer).ValidateBlock.func9
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/sync.go:888
  - expected final beacon entry in block to be at round 118327, got 81086:
    github.com/filecoin-project/lotus/chain/beacon.ValidateBlockValues
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/beacon/beacon.go:82


2020-10-04T17:40:47.513+0200    ERROR   chain   chain/sync_manager.go:421       sync error: collectChain failed:
    github.com/filecoin-project/lotus/chain.(*Syncer).Sync
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/sync.go:570
  - collectChain syncMessages:
    github.com/filecoin-project/lotus/chain.(*Syncer).collectChain
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/sync.go:1695
  - message processing failed:
    github.com/filecoin-project/lotus/chain.(*Syncer).syncMessagesAndCheckState.func1
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/sync.go:1471
  - validating block bafy2bzacechdx6xd62lcyy7rnyc4uxcxhuwqslcxfvj77fxlwafij3nhzchpy:
    github.com/filecoin-project/lotus/chain.(*Syncer).ValidateTipSet.func1
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/sync.go:620
  - 1 error occurred:
        * failed to validate blocks random beacon values:
    github.com/filecoin-project/lotus/chain.(*Syncer).ValidateBlock.func9
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/sync.go:888
  - expected final beacon entry in block to be at round 118327, got 81086:
    github.com/filecoin-project/lotus/chain/beacon.ValidateBlockValues
        /home/magik6k/github.com/filecoin-project/go-lotus/chain/beacon/beacon.go:82

@willscott
Copy link
Contributor

It looks like the chain up to block 51000 was using the incentinet for randomness.
While it doesn't need to contact the incentinet endpoint (I previously synced happily with no valid endpoints.) It will need the root of trust to validate that the beacons encoded in those previous chain blocks are valid.

@willscott willscott force-pushed the feat/drand-cloudflare branch from ad065da to 3209eb1 Compare October 5, 2020 00:57
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Is the test-vectors submodule update intentional?

Adding Cloudflare ensures additional redundancy.

Switching to HTTP for PL nodes reduces latency and cost. Randomness is
verified after being received.
@hsanjuan hsanjuan force-pushed the feat/drand-cloudflare branch from 3209eb1 to 8001ed3 Compare October 12, 2020 17:16
@hsanjuan
Copy link
Contributor Author

Is the test-vectors submodule update intentional?

pretty sure not, fixed.

@hsanjuan
Copy link
Contributor Author

Can't sync a fresh node with this PR applied, getting errors like

I'm syncing ok from scratch, at least 1300 rounds already, so I guess @willscott solved that issue.

@willscott
Copy link
Contributor

@magik6k this is ready for another merge attempt

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

Successfully merging this pull request may close these issues.

6 participants