all: protect self-mined block during reorg #17656
Conversation
rjl493456442
requested review from
holiman,
karalabe and
zsfelfoldi
as code owners
rjl493456442
force-pushed
the
miner-self-protect
branch
from
36ecd70
to
c4184c0
Compare
|
In Clique the coinbase is not the miner, rather a PoA signer vote. That said, the |
|
Also, I think we need a more flexible way to figure out which account we should whitelist. You could have multiple accounts available locally, each of which should be considered valid for whitelisting. In addition we also have the new |
rjl493456442
force-pushed
the
miner-self-protect
branch
3 times, most recently
from
fe3315b
to
887dab0
Compare
core/blockchain.go
Outdated
| if err != nil { | ||
| return NonStatTy, err | ||
| } | ||
| var currentLocal, newLocal bool |
There was a problem hiding this comment.
Nitpick: if you added currentAuthor and blockAuthor, then please also use currentLocal and blockLocal (instead of newLocal).
eth/backend.go
Outdated
| s.lock.RLock() | ||
| etherbase := s.etherbase | ||
| s.lock.RUnlock() | ||
| // Check whether the given address is etherbase. |
There was a problem hiding this comment.
Pleas move this comment to the top before the locking. That code segment is also already part of the check.
eth/backend.go
Outdated
| return true | ||
| } | ||
| // Check whether the given address in the keystore. | ||
| if wallets := s.AccountManager().Wallets(); len(wallets) > 0 { |
There was a problem hiding this comment.
You can access s.accountManager directly, no need for the function call.
eth/backend.go
Outdated
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm a bit reluctant about iterating over the account lists. This is generally an expensive operation. The transaction pool also did something similar and we had to remove it due to exchanges and other power users having hundreds of thousands of accounts, which we can't afford to iterate.
For the purpose of this PR (i.e. miners) I think it's fine to only check the etherbase and the txpool.locals. Those are the ones that are important anyway. For average nodes, this optimization is moot anyway since they won't mine nodes themselves.
rjl493456442
force-pushed
the
miner-self-protect
branch
from
887dab0
to
dbd3834
Compare
There was a problem hiding this comment.
Please fix the failing tests + linter. https://travis-ci.org/ethereum/go-ethereum/builds/429500728
PS: We'll need to upgrade to Go 1.11 to use the new gofmt.
rjl493456442
force-pushed
the
miner-self-protect
branch
2 times, most recently
from
516601f
to
bbb1b0c
Compare
There was a problem hiding this comment.
LGTM!
This PR makes Clique deadlocks a tiny tiny bit more probable (if signers keep going offline at the exact correct time and the remaining signers flood the network with blocks at the same time). Though I can't really see this being an realistic issue. Lets see. If it is indeed an issue, we can make the self-reorg choice only active for ethash but not for clique.
rjl493456442
force-pushed
the
miner-self-protect
branch
from
bbb1b0c
to
003a3a9
Compare
eth/backend.go
Outdated
| // r4 [X] | ||
| // In the round3, the inturn signer C is offline, so the worst case | ||
| // is A and D sign the block of round3 and reject the block of opponents | ||
| // and in the round4, B is offline, the whole network is stuck. |
There was a problem hiding this comment.
Clique requires 50%+1, so for 4 signers, it would need 3 to progress the chain.
rjl493456442
force-pushed
the
miner-self-protect
branch
from
003a3a9
to
fa29a45
Compare
eth/backend.go
Outdated
| // whether a block is mined by local accounts. We regard two types of | ||
| // accounts as local account: etherbase and accounts specified via | ||
| // `txpool.locals` flag. | ||
| func (s *Ethereum) isLocalAccount(addr common.Address) bool { |
There was a problem hiding this comment.
Perhaps lets call this isMinerAccount. It might be clearer that not all accounts are taken into consideration, rather only those that are considered miners.
eth/backend.go
Outdated
| // network is stuck. | ||
| if _, ok := s.engine.(*clique.Clique); ok { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Lets move this check into the blockchain when we do (or don't) the reorg. I really don't see why isLocalAccount should care about the consensus engine. That's up to the blockchain to consider when figuring out the reorg.
rjl493456442
force-pushed
the
miner-self-protect
branch
from
fa29a45
to
45464f2
Compare
rjl493456442
force-pushed
the
miner-self-protect
branch
from
45464f2
to
06f2676
Compare
|
@karalabe updated |
Still have to fix 2 issues.