Add support for mining ommers, up to 8 blocks behind the head job #2576
Conversation
Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com>
| import org.apache.tuweni.units.bigints.UInt256; | ||
|
|
||
| public class PoWSolver { | ||
|
|
||
| private static final long POW_JOB_TTL = 1000 * 60 * 5; // 5 minutes |
There was a problem hiding this comment.
is this value related to something ?
There was a problem hiding this comment.
Not really. 13s per block on average, so 13 times 8 is 108s, times 2 makes 3 minutes and a half, and buffered to 5 minutes to ensure we catch stragglers. We can use a different approach. I have another ticket to add meters to count accepted solutions vs rejected solutions.
There was a problem hiding this comment.
ok it was to make sure I didn't miss anything. did you had the opportunity to test it ? I can test it on ropsten if you want
There was a problem hiding this comment.
I did not. I kinda wanted folks to review as well. I added some unit tests to make sure it made sense.
There was a problem hiding this comment.
My instinct says this should be configurable. Probably via an --X flag.
There was a problem hiding this comment.
Is 5 minutes an ok default?
There was a problem hiding this comment.
maybe 2x high? But since we are just keeping block headers (right?) from 5 min ago unless things get very hairy I don't see it causing resource problems. If we were keeping TXes it could be problematic.
There was a problem hiding this comment.
Yep, only block headers. It should be fine.
| final Optional<PoWSolution> calculatedSolution = testNonce(inputs, solution.getNonce()); | ||
|
|
||
| if (calculatedSolution.isPresent()) { | ||
| LOG.debug("Accepting a solution from a miner"); | ||
| currentJob.get().solvedWith(calculatedSolution.get()); | ||
| currentJobs.remove(solution.getPowHash()); |
There was a problem hiding this comment.
Does the miner want multiple ommers minable? Removing on the first solution precludes that.
There was a problem hiding this comment.
Right now, we only accept one solution per block (the solveFor method blocks until the future completes). This repeats the same mechanism but with ommer blocks. We could potentially choose to allow more than one solution per block, turn those into ommers and replace the proposed block with the block with the best nonce. That's why I wanted a review, I'm just not sure how far to push this.
There was a problem hiding this comment.
To be clear, that'd be best addressed as a separate PR, because changing solveFor feels like a change worth doing on its own, whether we accept mining ommers or not.
|
Plumb the |
atoulme
force-pushed
the
add_ommer_mining_support
branch
from
814feaf to
ad44007
Compare
Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com>
atoulme
force-pushed
the
add_ommer_mining_support
branch
from
ad44007 to
fa02aa0
Compare
| import org.apache.tuweni.units.bigints.UInt256; | ||
|
|
||
| public class PoWSolver { | ||
|
|
||
| private static final int MAX_OMMER_DEPTH = 8; |
There was a problem hiding this comment.
For another PR, another --X candidate.
…perledger#2576) * Add support for mining ommers, up to 8 blocks behind the head job Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com> * Add a CLI option to set PoW jobs TTL Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com>
Signed-off-by: Antoine Toulme antoine@lunar-ocean.com
PR description
This allows miners to send solutions for blocks up to 8 blocks behind head, to be included as ommers.
Fixed Issue(s)
Fixes #2572
Changelog