WIP: try to enforce signing vs revocation #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devrandom
force-pushed
the
per-commitment
branch
from
0c98489 to
ebcddd9
Compare
devrandom
force-pushed
the
per-commitment
branch
4 times, most recently
from
950fa02 to
b661164
Compare
TheBlueMatt
reviewed
Jul 20, 2020
| if keys.per_commitment_point == self.inner.get_per_commitment_point(*revoked, secp_ctx) { | ||
| panic!("attempted to sign the latest revoked local commitment {}", self.inner.commitment_seed[0]); | ||
| } else { | ||
| panic!("can only sign the next two unrevoked commitment numbers, {} revoked={} point={}", |
There was a problem hiding this comment.
All the tests pass without this panic, and, indeed, I think you are correct in that it is just different objects getting serialized/read breaking things in the tests. It would be nice to enforce the other new assertions upstream even if we can't hit this one, but I think the solution is going to have to be some global static HashMap tracking revocation for a given commitment seed.
devrandom
force-pushed
the
per-commitment
branch
2 times, most recently
from
fefae6a to
b19d447
Compare
We had code in the router to support sending a payment via a single hop across channels exclusively provided by the next-/last-hop hints. However, in updating the fuzzer, I noted that this case not only didn't work, but paniced in some cases. Here, we both fix the panic, as well as write a new test which ensures we don't break support for such routing in the future.
In updating the router fuzzer, it discovered that a remote peer can cause us to overflow while multiplying the channel capacity value. Since the value should never exceed 21 million BTC, we just add a check for that.
These functions were created but previously not exported, however they are useful if we want to skip signature checking when accepting routing messages (which we really should be doing in the routing fuzzer).
It turns out (somewhat obviously) that expecting a fuzzer to correctly build multiple signatures which verify against multiple public keys in the same message was a bit too daunting, so we now skip message signatures in routing messages. We also take this opportunity to simplify the target itself somewhat, avoiding reading public keys over and over and instead generating routes to all the public keys that appeared in messages while running.
…r-fuzzer Make router_target a bit easier for fuzzers to explore and fix two found bugs
It seems travis isn't even bothering to spawn our jobs anymore, and Github CI has been pretty stable, so just drop Travis entirely.
devrandom
force-pushed
the
revoke-enforcement
branch
from
5936542 to
9f4e070
Compare
devrandom
force-pushed
the
revoke-enforcement
branch
from
9f4e070 to
0809d51
Compare
This makes the public utility methods in `NetworkGraph` able to do UTXO lookups ala `NetworkMsgHandler`'s `RoutingMessageHandler` implementation, slightly simplifying the public interface. We also take this opportunity to verify signatures before calling out to UTXO lookups, under the "do actions in order of cheapest-to-most-expensive to reduce DoS surface" principle.
This takes the now-public `NetworkGraph` message handling functions and splits them all into two methods - one which takes a required Secp256k1 context and verifies signatures and one which takes only the unsigned part of the message and does not take a Secp256k1 context. This both clarifies the public API as well as simplifies it, all without duplicating code. Finally, this adds an assertion in the Router fuzzer to make sure the constants used for message deserialization are correct.
…ngs-fix Split `NetworkGraph` message handling fns into unsigned and signed (+update bindings)
Bump version to 0.0.12
Bump lightning-net-tokio crate version.
Support for the gossip_queries feature flag (bits 6/7) is added to the Features struct. This feature is available in the Init and Node contexts. The gossip_queries feature is not fully implemented so this feature is disabled when sent to peers in the Init message.
To enable gossip_queries message decoding, this commit implements the wire module's Encoding trait for each message type. It also adds these messages to the wire module's Message enum and the read function to enable decoding of a buffer.
devrandom
force-pushed
the
revoke-enforcement
branch
from
0809d51 to
ba0dc14
Compare
The test vector was added in lightning/bolts#539.
devrandom
force-pushed
the
revoke-enforcement
branch
5 times, most recently
from
6d3889d to
06e198c
Compare
…-vectors Add BOLT 3 test vector for CLTV tiebreaker
devrandom
force-pushed
the
revoke-enforcement
branch
3 times, most recently
from
10a87ba to
ebc80c3
Compare
Closes lightningdevkit#766 Contributions by Devrandom <[email protected]>
…oject-context Add GLOSSARY.md and more
…eadme Update readme
…ment Signing the commitment transaction is almost always followed by signing the attached HTLC transactions, so fold the signing operations into a single method.
Signatures in OnChainTx must not fail, or we stand to lose funds
It is no longer optional since it is available at construction time.
…tlc-sigs Fold sign_holder_commitment_htlc_transactions into sign_holder_commitment
This allows stateful validation in EnforcingChannelKeys
We want to make sure that we don't sign revoked transactions. Given that ChannelKeys are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface.
devrandom
force-pushed
the
revoke-enforcement
branch
5 times, most recently
from
55f4ef8 to
f8e0405
Compare
When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic.
devrandom
force-pushed
the
revoke-enforcement
branch
from
f8e0405 to
142b0d6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fails in various places,
for example inbecause of persistence issues.ln::chanmon_update_fail_tests::during_funding_monitor_failIn particular,
EnforcingChannelKeyswould like to get some common state fromKeysInterfacewhen it's deserialized, but that's not available with the current persistence design.