Skip to content

light: run detector for sequentially validating light client#5538

Merged
melekes merged 4 commits intomasterfrom
anton/5445-light-detector-sequential
Nov 2, 2020
Merged

light: run detector for sequentially validating light client#5538
melekes merged 4 commits intomasterfrom
anton/5445-light-detector-sequential

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Oct 21, 2020

Closes #5445

@melekes melekes self-assigned this Oct 21, 2020
@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #5538 into master will increase coverage by 0.14%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #5538      +/-   ##
==========================================
+ Coverage   61.11%   61.25%   +0.14%     
==========================================
  Files         262      262              
  Lines       23685    23686       +1     
==========================================
+ Hits        14474    14509      +35     
+ Misses       7733     7701      -32     
+ Partials     1478     1476       -2     
Impacted Files Coverage Δ
light/detector.go 81.98% <ø> (ø)
light/client.go 68.55% <62.50%> (-0.26%) ⬇️
light/errors.go 80.00% <100.00%> (-3.34%) ⬇️
crypto/sr25519/pubkey.go 43.47% <0.00%> (-8.70%) ⬇️
p2p/switch.go 63.88% <0.00%> (-4.63%) ⬇️
mempool/reactor.go 85.60% <0.00%> (-2.28%) ⬇️
p2p/pex/pex_reactor.go 78.27% <0.00%> (-1.20%) ⬇️
consensus/state.go 68.59% <0.00%> (+0.37%) ⬆️
p2p/pex/addrbook.go 69.34% <0.00%> (+0.50%) ⬆️
blockchain/v0/reactor.go 61.57% <0.00%> (+0.98%) ⬆️
... and 8 more

@melekes melekes added the T:security Type: Security (specify priority) label Oct 22, 2020
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Yes I'm pretty sure this should work albeit potentially a rather costly operation for both the light client and the witness involved. 👍

I notice that we still have this check in place:

        // Validate the number of witnesses.
	if len(c.witnesses) < 1 && c.verificationMode == skipping {
		return nil, errNoWitnesses{}
	}

Do we want to extend this to sequential light clients requiring them to also have witnesses or not? Personally I'm leaning towards leaving it as is and not requiring sequential light clients to have witnesses because this is not a default setting and if people are changing the defaults then I'll assume they know what they are doing and also this attack is much more difficult than with skipping.

@melekes
Copy link
Contributor Author

melekes commented Nov 2, 2020

I notice that we still have this check in place:

good eye!

Do we want to extend this to sequential light clients requiring them to also have witnesses or not?

I'm in favor of simplifying the code in general. I will do it.

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

Labels

T:security Type: Security (specify priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

light: run detector for sequentially validating light client

2 participants