cmd, eth: add support for `--whitelist <blocknum>=<hash>` #18028

ryanschneider · 2018-11-02T21:05:03Z

As discussed with @holiman in #17935.

I'm not sure what the issue I was running into was last time where I thought I needed to do this for the bootnodes, I didn't run into that issue this time when testing.

I think the only remaining question I have is does this need to be implemented for light clients as well?

ryanschneider · 2018-11-02T21:09:00Z

FYI I've been testing this on ropsten with:

geth --testnet --blockhashwhitelists 4230605=0x8b3759c1daec512313671addeda9683c5a5b9442b8d6e8ca2d77d589cca3fce2 --blockhashwhitelists 4230000=0xbd9e8b9e8d8c00f8e0119cd996d2b665eec3dcd711a86b0681538fd9904c3f25 --verbosity 4

Those were the two blocks that came up during the fork issues as having bad alternatives.

holiman · 2018-11-02T22:52:03Z

Thanks! I'll take a deeper look and do some testing later, and I'm unsure about light clients, but one thing: I think it might actually be preferable to not have toml integration on this, since it not good if people use this temporary protection and then leave it in there permanently -- that will just increase the overall network noise.

ryanschneider · 2018-11-02T23:15:05Z

Good point, I was actually testing the toml integration and it didn't seem to work (dumpconfig didn't contain the map even though everything I've read about naomina/toml implied it should work. I'll update to explicitly ignore it for toml.

Also, I just took a stab at light client support here:
ryanschneider@6cd4c37

It seems to work, but I'll leave it outside this PR until we have more clarity if it's needed. One thing it did highlight though is that I defined BlockHashWhitelist in eth, I think that should be in a more common place higher up the hierarchy since it feels wrong for les to have to import eth just for that one type, so I'll make that change as well.

ryanschneider · 2018-11-02T23:26:04Z

Ok, I just force pushed a couple updates:

typo in flag name (it was --blockhashwhitelists plural, now singular)
moved eth.BlockHashWhitelist type to common.BlockHashWhitelist
Annotated the config w/ with toml:"-"

I'll update my light client branch to rebase off that, but won't open a PR for it just yet.

ryanschneider · 2018-11-03T00:21:11Z

And the light client branch has been force-pushed w/ the same changes: ryanschneider@85d7059

holiman

LGTM, but I'll do some testing on it aswell

eth/handler.go

ryanschneider · 2018-12-04T18:50:18Z

@holiman do you still need to test on your end?

holiman · 2018-12-04T20:46:45Z

Sorry, I totally forgot about this one. Looks good to me!

holiman · 2018-12-04T20:47:40Z

Could you rebase it to get a new travis run?

karalabe · 2018-12-05T12:41:19Z

geth --testnet --blockhashwhitelists 4230605=0x8b3759c1daec512313671addeda9683c5a5b9442b8d6e8ca2d77d589cca3fce2 --blockhashwhitelists 4230000=0xbd9e8b9e8d8c00f8e0119cd996d2b665eec3dcd711a86b0681538fd9904c3f25 --verbosity 4

AFAIK you can't specify the same argument multiple times, the former ones will be overwritten by later ones. We should use a comma separated vector for this.

Can we just name the flag --whitelist? I think that's enough without spelling out everything and the kitchen sink. Would be nice to introduce --blacklist too if we're going down this path.

ryanschneider · 2018-12-05T19:20:25Z

@karalabe I used cli.StringSliceFlag which does support sending it multiple times.

I'll change the flag to just whitelist and rebase it against current master.

ryanschneider · 2018-12-06T18:17:17Z

Any idea why the lint step on Tarvis is saying that eth/handler_test.go isn't gofmt'd?

karalabe · 2018-12-07T12:19:07Z

Any idea why the lint step on Tarvis is saying that eth/handler_test.go isn't gofmt'd?

@ryanschneider You're using Go 1.10, 1.11 changes some formatting rules. Please update your local installation

karalabe · 2018-12-07T12:19:58Z

cmd/utils/flags.go

@@ -182,6 +182,10 @@ var (
 		Name:  "lightkdf",
 		Usage: "Reduce key-derivation RAM & CPU usage at some expense of KDF strength",
 	}
+	BlockHashWhitelistsFlag = cli.StringSliceFlag{


I don't think we need such a long name for the variable either. WhitelistFlag should be enough (note, singular too).

We've also discussed this internally with @holiman and @fjl and decided that since all the flags that we have use comma separated value, we'd like this one to conform to the same style (even though you are right, this might allow setting the same flag multiple times).

Ok, I've force pushed updates as b406a1c57 that changes to a StringFlag.

karalabe · 2018-12-07T12:22:10Z

cmd/utils/flags.go

@@ -182,6 +182,10 @@ var (
 		Name:  "lightkdf",
 		Usage: "Reduce key-derivation RAM & CPU usage at some expense of KDF strength",
 	}
+	BlockHashWhitelistsFlag = cli.StringSliceFlag{
+		Name:  "whitelist",
+		Usage: "Reject peers which send non-matching block hashes (<block number>=<hash>)",


I think the description would be clearer as:

Comma separated block number-to-hash mappings to enforce (<number>=<hash>)

I don't think we need to mention peers and networking in this code. That's an implementation detail if you use the flag correctly.

Updated in b406a1c57 as well.

karalabe · 2018-12-07T12:24:46Z

common/blockhash_whitelist.go

@@ -0,0 +1,37 @@
+package common


We're trying in general to get rid of the common package. Lets not add further logic into it. Imho parsing the flags should just be part of the cmd/utils package as a private method. I also don't really see a reason to specify a custom typ for the whitelist. map[uint64]common.Hash is a pretty simple and understandable type.

Done in b406a1c57, map[uint64]common.Hash is used everywhere, no new files in common now.

I followed the convention seen in that file and added a new setWhitelist(ctx, cfg) func that does the parsing.

karalabe · 2018-12-07T12:25:00Z

eth/config.go

@@ -87,6 +87,9 @@ type Config struct {
 	SyncMode  downloader.SyncMode
 	NoPruning bool

+	// Block hash whitelist
+	BlockHashWhitelist *common.BlockHashWhitelist `toml:"-"`


BlockHashWhitelist -> Whitelist

There's no reason to use a pointer here. A map can be nil.

karalabe · 2018-12-07T12:26:37Z

eth/handler.go

@@ -88,6 +89,8 @@ type ProtocolManager struct {
 	txsSub        event.Subscription
 	minedBlockSub *event.TypeMuxSubscription

+	whitelist *common.BlockHashWhitelist


There's no reason to use a pointer here. A map can be nil.

karalabe · 2018-12-07T12:27:18Z

eth/handler.go

@@ -307,6 +311,18 @@ func (pm *ProtocolManager) handle(p *peer) error {
 			}
 		}()
 	}
+
+	// If we have any explicit whitelist block hashes, request them
+	if whitelist := pm.whitelist; whitelist != nil {


No reason to create this whole assign-the-validate check if you don't use a pointer. A map can be nil, and you can iterate a nil map (will be a noop).

karalabe · 2018-12-07T12:27:22Z

eth/handler.go

@@ -452,6 +468,18 @@ func (pm *ProtocolManager) handleMsg(p *peer) error {
 		// Filter out any explicitly requested headers, deliver the rest to the downloader
 		filter := len(headers) == 1
 		if filter {
+			// Check for any responses not matching our whitelist
+			if whitelist := pm.whitelist; whitelist != nil {


No reason to create this whole assign-the-validate check if you don't use a pointer. A map can be nil, and you can iterate a nil map (will be a noop).

karalabe · 2018-12-07T12:29:56Z

eth/handler.go

+				if expected, ok := (*whitelist)[headers[0].Number.Uint64()]; ok {
+					actual := headers[0].Hash()
+					if !bytes.Equal(expected.Bytes(), actual.Bytes()) {
+						p.Log().Debug("dropping peer with non-matching whitelist block", "number", headers[0].Number.Uint64(), "expected", expected, "actual", actual)


Please capitalize log messages: Dropping

Instead of actual use hash and move it as the second context parameter after number.

karalabe · 2018-12-07T12:31:15Z

eth/handler.go

+						return errors.New("whitelist block mismatch")
+					}
+
+					p.Log().Debug("whitelist match", "number", headers[0].Number.Uint64(), "expected", expected)


Please drop the empty line, no use for it. Please make the log message meaningful.

"Whitelist block verified", Please use hash, not "expected". It's a convention throughout the code.

karalabe · 2018-12-07T12:31:32Z

eth/handler_test.go

+		{1, nil, nil, 1},                                                         // A single random block should be retrievable
+		{10, nil, nil, 10},                                                       // Multiple random blocks should be retrievable
+		{limit, nil, nil, limit},                                                 // The maximum possible blocks should be retrievable
+		{limit + 1, nil, nil, limit},                                             // No more than the possible block count should be returned


You'll need Go 1.11 to not break this gofmt things.

Weird, I've been using Go 1.11 for awhile, but somehow my IDE was still using 1.10 for go fmt. I re-setup the go fmt support and confirmed it's using Go 1.11 style now, so these changes weren't made in the new commit.

ryanschneider · 2018-12-07T19:14:02Z

Ok, I think all of @karalabe concerns have been addressed, sorry if that spammed you with individual updates, I just realized that since I'm not a reviewer they went out one-by-one instead of in a batch. I left the comments unresolved so you can decide if the changes resolve them, though in general I think GH's UX for handling resolution on outdated code leaves a little to be desired, if you have any feedback on how you want me to communicate changes in future PRs I'm all ears.

cmd/utils/flags.go

eth/handler.go

* Rejects peers that respond with a different hash for any of the passed in block numbers. * Meant for emergency situations when the network forks unexpectedly.

karalabe

LGTM

ryanschneider requested a review from karalabe as a code owner November 2, 2018 21:05

ryanschneider force-pushed the blockhash-whitelist branch from 0f2da54 to d052166 Compare November 2, 2018 23:23

ryanschneider force-pushed the blockhash-whitelist branch from d052166 to 4633346 Compare November 2, 2018 23:29

holiman added the ropsten-lessons label Nov 3, 2018

holiman reviewed Nov 3, 2018

View reviewed changes

eth/handler.go Outdated Show resolved Hide resolved

holiman approved these changes Dec 4, 2018

View reviewed changes

ryanschneider force-pushed the blockhash-whitelist branch from a069613 to 654e545 Compare December 5, 2018 19:24

karalabe requested changes Dec 7, 2018

View reviewed changes

karalabe added this to the 1.8.20 milestone Dec 7, 2018

ryanschneider force-pushed the blockhash-whitelist branch from 654e545 to b406a1c Compare December 7, 2018 19:05

ryanschneider changed the title ~~cmd, eth: Add support for --blockhashwhitelists flag to explicitly whitelist chains containing certain blocks~~ cmd, eth: Add support for --whitelist <blocknum>=<hash>,... flag Dec 7, 2018

ryanschneider commented Dec 7, 2018

View reviewed changes

cmd/utils/flags.go Outdated Show resolved Hide resolved

ryanschneider force-pushed the blockhash-whitelist branch from b406a1c to 4984ad9 Compare December 7, 2018 19:19

ryanschneider commented Dec 7, 2018

View reviewed changes

eth/handler.go Outdated Show resolved Hide resolved

ryanschneider force-pushed the blockhash-whitelist branch from 4984ad9 to 1f84480 Compare December 7, 2018 19:22

ryanschneider and others added 2 commits December 10, 2018 14:30

cmd, eth: Add support for --whitelist <blocknum>=<hash>,... flag

48b70ec

* Rejects peers that respond with a different hash for any of the passed in block numbers. * Meant for emergency situations when the network forks unexpectedly.

cmd/utils, eth: minor polishes on whitelist code

31b3334

karalabe force-pushed the blockhash-whitelist branch from 1f84480 to 31b3334 Compare December 10, 2018 12:47

karalabe approved these changes Dec 10, 2018

View reviewed changes

karalabe changed the title ~~cmd, eth: Add support for --whitelist <blocknum>=<hash>,... flag~~ cmd, eth: add support for --whitelist <blocknum>=<hash>,... flag Dec 10, 2018

karalabe changed the title ~~cmd, eth: add support for --whitelist <blocknum>=<hash>,... flag~~ cmd, eth: add support for --whitelist <blocknum>=<hash> Dec 10, 2018

karalabe merged commit 9fe5d20 into ethereum:master Dec 10, 2018

holiman mentioned this pull request Jan 11, 2019

feature request: argument for bad blocks blacklist to prevent fast-syncing to an incorrect chain #17935

Closed

ryanschneider mentioned this pull request Mar 4, 2022

cmd/geth: rename --whitelist to --eth.requiredblocks #24505

Merged

cmd, eth: add support for --whitelist <blocknum>=<hash> #18028

cmd, eth: add support for --whitelist <blocknum>=<hash> #18028

Conversation

ryanschneider commented Nov 2, 2018

ryanschneider commented Nov 2, 2018

holiman commented Nov 2, 2018

ryanschneider commented Nov 2, 2018

ryanschneider commented Nov 2, 2018

ryanschneider commented Nov 3, 2018

holiman left a comment

Choose a reason for hiding this comment

ryanschneider commented Dec 4, 2018

holiman commented Dec 4, 2018

holiman commented Dec 4, 2018

karalabe commented Dec 5, 2018

ryanschneider commented Dec 5, 2018

ryanschneider commented Dec 6, 2018

karalabe commented Dec 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanschneider commented Dec 7, 2018

karalabe left a comment

Choose a reason for hiding this comment

cmd, eth: add support for `--whitelist <blocknum>=<hash>` #18028

cmd, eth: add support for `--whitelist <blocknum>=<hash>` #18028