Cache extra data #1389
Cache extra data #1389
Conversation
…BlockHeader. Currently it almost always winds up as an unparsed extra data but we can now move towards creating a parsed form upfront.
…e proposer address on demand without any additional input.
…ieved directly from the header.
| @@ -40,8 +40,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
off topic - but this should be renamed to something not containing "Interface"
| if (inputExtraData instanceof CliqueExtraData) { | ||
| return (CliqueExtraData) inputExtraData; | ||
| } | ||
| LOG.warn( |
There was a problem hiding this comment.
is this a warning? Or is it expected that someone MUST call decodeRaw quite early in the reception (why not force the call during blockimporter?)
There was a problem hiding this comment.
I would expect to never see this message because the extra data should only ever be parsed to the right type. But the nice thing about this approach is that if somehow we get that wrong, things we still work, just with a performance penalty (and this warning is intended to make that situation visible).
.../src/main/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueMinerExecutor.java
Outdated
Show resolved
Hide resolved
...c/test/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueBlockCreatorTest.java
Outdated
Show resolved
Hide resolved
…ta of the wrong type.
| final CliqueExtraData extraData = new CliqueExtraData(vanityDataToInsert, null, validators); | ||
|
|
||
| return extraData.encode(); | ||
| return new CliqueExtraData(vanityDataToInsert, null, validators, null).encode(); |
There was a problem hiding this comment.
What about adding a static encode() method, so you don't have to pass a null header? Then you can run checkNotNull on header in the CliqueExtraData constructor.
| @@ -47,9 +57,28 @@ public CliqueExtraData( | |||
| this.vanityData = vanityData; | |||
| this.proposerSeal = Optional.ofNullable(proposerSeal); | |||
| this.validators = validators; | |||
| proposerAddress = | |||
| Suppliers.memoize(() -> CliqueBlockHashing.recoverProposerAddress(header, this)); | |||
There was a problem hiding this comment.
When header is null, looks like getProposerAddress() will throw an error
There was a problem hiding this comment.
So all these nulls are required because when you're mining a block you need to have something for the extra data before you can calculate the hash and sign it. Except for one slightly contrived case in test setup, whenever header is null, proposerSeal is also null or empty - so in either case you can't recover the proposer address because there isn't one.
But I like the idea of a static encode function and it fits in quite well so have done that. You can still wind up with a CliqueExtraData instance with an invalid principal seal but we need to handle that because that's what the genesis block has.
PR description
Parse extra data into a consensus protocol specific type when creating a block header. Avoids reprising the extra data all the time and gives a hook to cache things like the proposer seal which are expensive to compute.