Skip to content

Conversation

TheBlueMatt
Copy link
Collaborator

Val noted in 748 (#748 (comment)) that exposing the existing functions as public was a bit lazy. I indicated I'd handle it in a follow-up post-0.0.12, but realized that 0.0.12 didn't actually pass bindings checks because the newly-exposed APIs required additional features in the bindings. Thus, I went ahead and did the fixup here as well as updated the auto-generated bindings successfully.

As a bonus, this drops Travis CI.

It seems travis isn't even bothering to spawn our jobs anymore, and
Github CI has been pretty stable, so just drop Travis entirely.
@TheBlueMatt TheBlueMatt added this to the 0.0.12 milestone Nov 24, 2020
@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #758 (ccd4a7a) into main (fc7df54) will increase coverage by 0.00%.
The diff coverage is 90.58%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #758   +/-   ##
=======================================
  Coverage   91.41%   91.41%           
=======================================
  Files          37       37           
  Lines       22411    22423   +12     
=======================================
+ Hits        20486    20497   +11     
- Misses       1925     1926    +1     
Impacted Files Coverage Δ
lightning/src/routing/network_graph.rs 91.30% <90.58%> (-0.19%) ⬇️
lightning/src/ln/functional_tests.rs 97.17% <0.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc7df54...ccd4a7a. Read the comment docs.

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.
@TheBlueMatt TheBlueMatt force-pushed the 2020-11-bindings-fix branch 2 times, most recently from 932d3ac to 083c02a Compare November 24, 2020 19:31
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean-up! Looks like a largely mechanical change.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new API!

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.
@TheBlueMatt
Copy link
Collaborator Author

Only changed the minor nits from @jkczyz.

@TheBlueMatt TheBlueMatt merged commit 84716ff into lightningdevkit:main Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants