Ibft now handles failing sign operations #831
Conversation
rain-on
force-pushed
the
sign_failure
branch
4 times, most recently
from
b8e917b to
00b7f80
Compare
| final Prepare localPrepareMessage = | ||
| messageFactory.createPrepare(getRoundIdentifier(), block.getHash()); | ||
| peerIsPrepared(localPrepareMessage); | ||
| transmitter.multicastPrepare( |
There was a problem hiding this comment.
you have changed the operations order from previous logic? i.e. Previously it was multiCastPrepare then peerIsPrepared. Now it is peerIsPrepared then multicastPrepare? Is that by mistake or it doesn't matter?
There was a problem hiding this comment.
that shouldn't matter in this instance.
|
|
||
| final ProposalMessageData message = ProposalMessageData.create(data); | ||
| final ProposalMessageData message = ProposalMessageData.create(data); |
There was a problem hiding this comment.
The try/catch only needs to be around the messageFactory.create method
There was a problem hiding this comment.
It could be thinner, but didn't seem to add a lot of value to do so.
| } | ||
|
|
||
| public void multicastPrepare(final ConsensusRoundIdentifier roundIdentifier, final Hash digest) { | ||
| final Prepare data = messageFactory.createPrepare(roundIdentifier, digest); | ||
| try { | ||
| final Prepare data = messageFactory.createPrepare(roundIdentifier, digest); |
There was a problem hiding this comment.
Same as above for the all the other messages in the message transmitter
There was a problem hiding this comment.
agreed - but so long as we don't transmit, then we've done what we need to.
| localPrepareMessage.getRoundIdentifier(), localPrepareMessage.getDigest()); | ||
| peerIsPrepared(localPrepareMessage); | ||
| try { | ||
| final Prepare localPrepareMessage = |
There was a problem hiding this comment.
Is it only the createPrepare that can fail with a SecurityModuleException? Don't think this try/catch needs to be around the peerIsPrepared or multicastPrepare
| roundState.addCommitMessage(localCommitMessage); | ||
| } catch (final SecurityModuleException e) { | ||
| LOG.warn("Failed to create signed Commit message; {}", e.getMessage()); | ||
| return blockAccepted; |
There was a problem hiding this comment.
Should we be returning true in this case, as this will result in a prepare message being created?
There was a problem hiding this comment.
From where this is called - the return value implies the block is good, and that this node should try and send a prepare message - which, regardless of this failure, is still true.
|
What should happen with the SecurityModule.getPublicKey? It can also fail with a SecurityModuleException, perhaps it shouldn't though? |
Tests are not yet written, however IbftRound has been updated to accept Signing errors (eg no signature supplier available) and continue operating if possible. This also catches failures in signing and ECDH Key agreement creation during discovery and handshaking. Signed-off-by: Trent Mohay <trent.mohay@consensys.net>
IbftRound has been updated to handle Signing errors (eg no signature supplier available) and
continue operating if possible.
Signed-off-by: Trent Mohay trent.mohay@consensys.net