Skip to content
This repository has been archived by the owner on May 22, 2023. It is now read-only.

Add signer publication delay #616

Merged
merged 7 commits into from
Nov 23, 2020
Merged

Add signer publication delay #616

merged 7 commits into from
Nov 23, 2020

Conversation

lukasz-zimnoch
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch commented Nov 19, 2020

Closes: #553
Depends on: #610

This PR introduces a delay mechanism that forces signers to wait an amount of time before publishing the signature for a given keep. The value of the delay is the product of the signer index and a constant step (5 minutes). So:

  • member 0 doesn't wait and start the publication process immediately
  • member 1 waits 5 minutes
  • member 2 waits 10 minutes

This change should prevent signers to publish the signature at the same time. Publishing at the same time is not optimal as only the first transaction succeeds while the second and third revert and burn some gas.

Each signer should wait an amount of time
before publishing the signature for given keep.
The exact amount of time is determined basing
on signer index. This should prevent
signers to publish the signature in the same
time and burn gas for transactions which will
be reverted.
pkg/node/node.go Outdated Show resolved Hide resolved
pkg/node/node.go Outdated Show resolved Hide resolved
pkg/node/node.go Outdated Show resolved Hide resolved
pkg/node/node.go Outdated Show resolved Hide resolved
pkg/node/node.go Outdated Show resolved Hide resolved
pkg/node/node.go Outdated Show resolved Hide resolved
@pdyraga pdyraga added this to the v1.5.0 milestone Nov 20, 2020
Base automatically changed from chain-confirmations to master November 20, 2020 15:23
@lukasz-zimnoch lukasz-zimnoch marked this pull request as ready for review November 23, 2020 09:05
@pdyraga
Copy link
Member

pdyraga commented Nov 23, 2020

We should now remove two logs saying signature for keep [...] already submitted. See what's happening in logs when the client is not the one that is submitting a signature:

2020-11-23T11:22:10.222+0100	INFO	tss-lib	party {1,}: signing finished!
2020-11-23T11:22:10.226+0100	INFO	keep-ecdsa	waiting [5m0s] before publishing signature for keep [0xe1C664a63BD1D017270aaE9bda45A776bA86a078]
2020-11-23T11:27:10.432+0100	INFO	keep-ecdsa	confirming on-chain signature submission for keep [0xe1C664a63BD1D017270aaE9bda45A776bA86a078]
2020-11-23T11:27:10.432+0100	INFO	keep-chainutil	waiting for block [17807] to confirm chain state
2020-11-23T11:29:45.889+0100	INFO	keep-ecdsa	signature for keep [0xe1C664a63BD1D017270aaE9bda45A776bA86a078] successfully submitted and confirmed on-chain
2020-11-23T11:29:45.889+0100	INFO	keep-ecdsa	signature for keep [0xe1C664a63BD1D017270aaE9bda45A776bA86a078] already submitted: [50b2c43fd39106bafbba0da34fc430e1f91e3c96ea2acee2bc34119f92b37750]

We do not need the last log, the one saying signature for keep [0xe1C664a63BD1D017270aaE9bda45A776bA86a078] successfully submitted and confirmed on-chain is enough. See it's emitted in confirmSignature function, so for the two cases I mentioned, we should be fine with removing logger.Info and leaving there just return.

@pdyraga
Copy link
Member

pdyraga commented Nov 23, 2020

When testing this PR, I noticed the party ID logged during TSS signing is different from signer index returned from the chain. I do not think it is blocking for this PR as the problem lies elsewhere so I opened an issue to capture it: #622

@pdyraga
Copy link
Member

pdyraga commented Nov 23, 2020

Scenarios tested:

  • Happy path, signer with on-chain index 0 submits a signature to the chain, other clients confirm and exit the publication process.
  • Signer with on-chain index 0 does not submit a signature to the chain, signer with on-chain index 1 does it after a delay. All signers confirm the signature on the chain and exit the publication process.
  • Signer with on-chain index 0 does not submit the signature, all clients get restarted. Once they all appear online again, they start the signing process, publish the signature, confirm it on-chain, and finish the signing process gracefully.

@pdyraga
Copy link
Member

pdyraga commented Nov 23, 2020

A nice improvement to the code presented here would be to start the confirmation process if a signature appeared on the chain despite the fact the client is in the "delay publication" state. I do not think it's worth blocking because of that, and we can revisit this once we implement a new interface for event subscriptions (see keep-network/keep-core#491).

Captured it in the issue: #623

@pdyraga pdyraga merged commit dd08d60 into master Nov 23, 2020
@pdyraga pdyraga deleted the signature-publication-delays branch November 23, 2020 12:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECDSA client is a little too aggressive about submitting the transaction signature.
2 participants