Generate local signatures with additional randomness #2205
Conversation
wpaulino
force-pushed
the
sign-ecdsa-with-noncedata
branch
2 times, most recently
from
b8d85c9
to
099d875
Compare
| /// because we are about to broadcast a holder transaction. | ||
| pub fn sign<T: secp256k1::Signing>(&self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) -> Signature { | ||
| /// Signs the counterparty's commitment transaction. | ||
| pub fn sign_counterparty_commitment<T: secp256k1::Signing>(&self, funding_key: &SecretKey, funding_redeemscript: &Script, channel_value_satoshis: u64, secp_ctx: &Secp256k1<T>) -> Signature { |
There was a problem hiding this comment.
I mean, this is fine, but is there a reason to break this out and treat them separately?
There was a problem hiding this comment.
We have a few test assertions that rely on the same commitment_signed being resent after a channel_reestablish. This is also the case for closing_signed signatures. We could remove those assertions I guess, since the protocol does allow you to retransmit a different signature after a reconnection.
There was a problem hiding this comment.
Ah, okay, yea, that's fine then, I think, I just wanted to check that there's some reason.
There was a problem hiding this comment.
Sorry, fine as in we can remove the assertions or just leave them as is?
There was a problem hiding this comment.
Let's leave it as-is.
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2205 +/- ##
==========================================
+ Coverage 91.38% 91.48% +0.10%
==========================================
Files 102 104 +2
Lines 50832 51176 +344
Branches 50832 51176 +344
==========================================
+ Hits 46451 46820 +369
+ Misses 4381 4356 -25
... and 53 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
This allows the `InMemorySigner` to produce its own randomness, which we plan to use when generating signatures in future work. We can no longer derive `Clone` due to the `AtomicCounter`, so we opt to implement it manually.
Previously, our local signatures would always be deterministic, whether we'd grind for low R value signatures or not. For peers supporting SegWit, Bitcoin Core will generally use a transaction's witness-txid, as opposed to its txid, to advertise transactions. Therefore, to ensure a transaction has the best chance to propagate across node mempools in the network, each of its broadcast attempts should have a unique/distinct witness-txid, which we can achieve by introducing random nonce data when generating local signatures, such that they are no longer deterministic.
Gossip messages always use signatures in their compact form, so grinding for low R signatures is unnecessary.
To match the local signatures found in test vectors, we must make sure we don't use any additional randomess when generating signatures, as we'll arrive at a different signature otherwise.
wpaulino
force-pushed
the
sign-ecdsa-with-noncedata
branch
from
099d875
to
86531e5
Compare
Previously, our local signatures would always be deterministic, whether we'd grind for low R value signatures or not. For peers supporting SegWit, Bitcoin Core will generally use a transaction's witness-txid, as opposed to its txid, to advertise transactions. Therefore, to ensure a transaction has the best chance to propagate across node mempools in the network, each of its broadcast attempts should have a unique/distinct witness-txid, which we can achieve by introducing random nonce data when generating local signatures, such that they are no longer deterministic.