main: verify network pow limits by JFixby · Pull Request #1302 · decred/dcrd

JFixby · 2018-06-15T12:09:43Z

1. Add test to check `PowLimit` and `PowLimitBits` parameters of the network

Network PoW limit has two different representations of the same difficulty value. See example at the chaincfg/mainnetparams.go:

PowLimit:         mainPowLimit,// big int
PowLimitBits:     0x1d00ffff,  //conceptually the same
                               //value, but in an obscure form

This test ensures they properly match.

2. Add test to check genesis block respects consensus rules

Genesis block difficulty should be within the network PoW limit. See chaincfg/genesis.go:

var genesisBlock = wire.MsgBlock{
	Header: wire.BlockHeader{
		...
		Bits:         0x1b01ffff, // Is this a valid value?
		...
	},
}

davecgh

The extra tests are welcome, but please put them in blockchain, not chaincfg. This would introduce a dependency from chaincfg on blockchain, which as previously discussed, we do not want.

davecgh · 2018-06-15T17:56:05Z

networkparams_test.go

It's 2018 and this is new code for Decred, so there shouldn't be any copyright attributed to btcsuite.

davecgh · 2018-06-15T17:56:43Z

networkparams_test.go

Should have a space after the // and end with a period to be consistent with the rest of the code.

davecgh · 2018-06-15T17:57:07Z

networkparams_test.go

Superfluous blank line.

davecgh · 2018-06-15T17:57:23Z

networkparams_test.go

Superfluous blank line.

davecgh · 2018-06-15T17:58:43Z

networkparams_test.go

Please define functions before you call them. People read English left to right, top to bottom and thus that transfers to reading code as well. It's incredibly annoying to have to bounce around a file when reading code because the functions are defined after they're called.

davecgh · 2018-06-15T18:01:03Z

networkparams_test.go

Superfluous blank line.

davecgh · 2018-06-15T18:02:47Z

networkparams_test.go

80 col limit as per rest of the code.

davecgh · 2018-06-15T18:02:57Z

networkparams_test.go

80 col limit as per rest of the code.

davecgh · 2018-06-15T18:03:00Z

networkparams_test.go

Tests should generally be silent. If you really want to print something out, it would need to be via t.Log/t.Logf.

davecgh · 2018-06-15T18:03:52Z

networkparams_test.go

Lacks space after // and does not end with a period. Several other instances of this as well.

davecgh · 2018-06-15T18:12:54Z

Also, when you update the PR, you'll need to amend the commit to make it follow the code contribution guildes for commit messages as this one doesn't. The PR title is close (wrong subsystem), but it's the commit itself that is more important since that is what forms history.

JFixby · 2018-06-15T18:29:46Z

This new file networkparams_test.go currently is in the root of the project. This way it can access both blockchain and chaincfg without any complications. Should I keep it in there?

fmt.Println is silent when test passed and prints stuff only when test failed. That is exactly what I want to achieve here. Should I still leave it as is or should I change it to the t.Log?

davecgh · 2018-06-15T18:40:24Z

Oh, I thought I saw it in chaincfg. It's fine in the root of the project.

JFixby · 2018-06-15T18:42:06Z

go fmt doesn't bend this line automatically.
What would be the right way to format it respecting the 80 chars limit?

davecgh · 2018-06-15T18:48:56Z

Yeah, gofmt does not enforce a max column width, but all of the code in the dcrd repo does (minus what I'm sure are a few exceptions that have snuck in over time).

It would look like:

		t.Fatalf("Genesis block fails the consensus: genesis.Header.Bits=%v "+
			"should respect network PoW limit: %v", fmt.Sprintf("%x", bits),
			fmt.Sprintf("%064x", powLimitBigInt))

EDIT: That said, t.Fatalf already supports formatting. There is no need to call fmt.Sprintf again.

		t.Fatalf("Genesis block fails the consensus: genesis.Header.Bits=%x "+
			"should respect network PoW limit: %064x", bits, powLimitBigInt)

davecgh · 2018-06-15T18:53:42Z

The fmt.Println stuff is not silent when you run with go test -v which most tooling, including Travis, does. It shows up out of order with the tests. You can see that by looking at the Travis logs. That said, I'm not sure why you want to print it out anyway. They're tests to ensure accuracy and the t.Fatal statements handle printing the pertinent information in the case of failure.

davecgh · 2018-06-15T18:41:02Z

networkparams_test.go

This code didn't exist in 2015. It's brand new. It should only be 2018.

davecgh · 2018-06-15T18:56:55Z

networkparams_test.go

No need to call fmt.Sprintf again since t.Fatalf already supports formatting.

e.g.

t.Fatalf("PowLimit values mismatch: params.PowLimit=%064x is "+ "inconsistent with params.PowLimitBits=%x", powLimitBigInt, powLimitCompact)

davecgh · 2018-06-15T18:57:42Z

networkparams_test.go

Same thing here:

t.Fatalf("Genesis block fails the consensus: genesis.Header.Bits=%x "+ "should respect network PoW limit: %064x", bits, powLimitBigInt)

JFixby · 2018-06-15T19:16:22Z

The following code:

                t.Fatalf("genesis.Header.Bits:%x", bits)
		t.Fatalf("                   :%064x", bitsAsBigInt) // converted to big.In
		t.Fatalf("params.PowLimit    :%064x", powLimitBigInt)
		t.Fatalf("")
		t.Fatalf("Genesis block fails the consensus: "+
			"genesis.Header.Bits=%x should respect network PoW limit: "+
			"%064x", bits, powLimitBigInt)

Prints only the first line:

--- FAIL: TestDecredNetworkSettings (0.00s)
	D:\GOWS\src\github.com\decred\dcrd\networkparams_test.go:53: genesis.Header.Bits:1b01ffff
FAIL
FAIL	github.com/decred/dcrd	0.459s
Error: Tests failed.

This code:

    fmt.Println("genesis.Header.Bits:", fmt.Sprintf("%x", bits)) 
    fmt.Println("                   :", fmt.Sprintf("%064x", bitsAsBigInt)) // converted to big.In 
    fmt.Println("params.PowLimit    :", fmt.Sprintf("%064x", powLimitBigInt)) 
    fmt.Println() 
    t.Fatalf("Genesis block fails the consensus: "+
			"genesis.Header.Bits=%x should respect network PoW limit: "+
			"%064x", bits, powLimitBigInt)

Prints detailed info for debug:

genesis.Header.Bits: 1b01ffff
                   : 000000000001ffff000000000000000000000000000000000000000000000000
params.PowLimit    : 00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff

--- FAIL: TestDecredNetworkSettings (0.00s)
	D:\GOWS\src\github.com\decred\dcrd\networkparams_test.go:58: Genesis block fails the consensus: genesis.Header.Bits=1b01ffff should respect network PoW limit: 00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff

The idea is that developer can use this information for debug when he fails the test on his laptop.

I see go test -v prints also fmt.Println, thank you.

Still I keep in inside the fail condition, so it is not printed when not necessary.

JFixby · 2018-06-15T19:41:23Z

Ok I removed fmt.Println. Should be good now.

davecgh · 2018-06-15T20:23:14Z

networkparams_test.go

Exceeds 80 cols.

davecgh · 2018-06-15T20:23:20Z

networkparams_test.go

Exceeds 80.

davecgh · 2018-06-15T20:24:26Z

networkparams_test.go

Please don't postfix comments. It's inconsistent with 99.9% of the codebase.

davecgh · 2018-06-15T20:26:41Z

networkparams_test.go

This is not really true, since the genesis block is valid by definition. That means it doesn't actually have to respect the difficulty, and in fact, if you notice, the genesis block is not solved.

Add tests to ensure: - network PowLimit and PowLimitBits actually match - genesis block respects network PoW limit

JFixby · 2018-06-15T21:40:04Z

I updated the comments:

+// checkGenesisBlockRespectsNetworkPowLimit ensures genesis.Header.Bits value
+// is within the network PoW limit.
+//
+// Genesis header bits define starting difficulty of the network.
+// Header bits of each block define target difficulty of the subsequent block.
+//
+// The first few solved blocks of the network will inherit the genesis block
+// bits value before the difficulty reajustment takes place.
+//
+// Solved block shouldn't be rejected due to the PoW limit check.
+//
+// This test ensures these blocks will respect the network PoW limit.

JFixby · 2018-06-15T22:00:08Z

@davecgh Thank you for help.

JFixby force-pushed the check_network_params branch from 5c217fb to 40a5fe8 Compare June 15, 2018 12:20

JFixby changed the title ~~chaincfg: verify network pow limits~~ main: verify network pow limits Jun 15, 2018

davecgh requested changes Jun 15, 2018

View reviewed changes

JFixby force-pushed the check_network_params branch 2 times, most recently from 86509b4 to e8ca9c0 Compare June 15, 2018 18:34

JFixby force-pushed the check_network_params branch from e8ca9c0 to c0d3409 Compare June 15, 2018 18:50

davecgh requested changes Jun 15, 2018

View reviewed changes

JFixby force-pushed the check_network_params branch 3 times, most recently from 9bf2093 to 1a2ba11 Compare June 15, 2018 19:38

davecgh requested changes Jun 15, 2018

View reviewed changes

main: verify network pow limits

42a3cfe

Add tests to ensure: - network PowLimit and PowLimitBits actually match - genesis block respects network PoW limit

JFixby force-pushed the check_network_params branch from 1a2ba11 to 42a3cfe Compare June 15, 2018 21:38

davecgh approved these changes Jun 15, 2018

View reviewed changes

davecgh merged commit d7e0a3b into decred:master Jun 15, 2018

JFixby deleted the check_network_params branch June 17, 2018 10:44

Conversation

JFixby commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Add test to check PowLimit and PowLimitBits parameters of the network

2. Add test to check genesis block respects consensus rules

Uh oh!

davecgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davecgh commented Jun 15, 2018

Uh oh!

JFixby commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davecgh commented Jun 15, 2018

Uh oh!

JFixby commented Jun 15, 2018

Uh oh!

davecgh commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davecgh commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davecgh Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JFixby commented Jun 15, 2018

Uh oh!

JFixby commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JFixby commented Jun 15, 2018

Uh oh!

JFixby commented Jun 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JFixby commented Jun 15, 2018 •

edited

Loading

1. Add test to check `PowLimit` and `PowLimitBits` parameters of the network

JFixby commented Jun 15, 2018 •

edited

Loading

davecgh commented Jun 15, 2018 •

edited

Loading

davecgh Jun 15, 2018 •

edited

Loading