Pre-C bindings cleanups (2) #638

TheBlueMatt · 2020-06-11T19:37:59Z

A second batch of assorted code cleanups that make C binding generation a bit easier. See individual commits for details, but they're all pretty straight forward.

codecov · 2020-06-11T20:20:58Z

Codecov Report

Merging #638 into master will decrease coverage by 0.07%.
The diff coverage is 89.54%.

@@            Coverage Diff             @@
##           master     #638      +/-   ##
==========================================
- Coverage   91.30%   91.23%   -0.08%     
==========================================
  Files          35       35              
  Lines       21104    21114      +10     
==========================================
- Hits        19270    19263       -7     
- Misses       1834     1851      +17

Impacted Files	Coverage Δ
lightning/src/ln/msgs.rs	`90.03% <ø> (ø)`
lightning/src/util/ser.rs	`87.45% <ø> (ø)`
lightning/src/util/test_utils.rs	`85.20% <0.00%> (ø)`
lightning/src/routing/network_graph.rs	`91.06% <38.09%> (-0.56%)`	⬇️
lightning/src/ln/channel.rs	`86.76% <75.00%> (-0.03%)`	⬇️
lightning/src/chain/chaininterface.rs	`91.93% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/keysinterface.rs	`94.78% <100.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.45% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`85.45% <100.00%> (ø)`
lightning/src/ln/channelmonitor.rs	`95.70% <100.00%> (+<0.01%)`	⬆️
... and 5 more

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 70fca07...5c37023. Read the comment docs.

jkczyz

The changes related to get_funding_txo in 0acdc55 belong in 897a1ab.

jkczyz · 2020-06-12T23:22:39Z

lightning/src/ln/channelmonitor.rs

+		{
+			let funding_txo = monitor.get_funding_txo();


Is this scope necessary because of the borrow here?

It is for 1.22, but not for newer versions.

jkczyz · 2020-06-12T23:34:31Z

lightning/src/chain/chaininterface.rs

 		let mut matched_index = Vec::new();
 		{
 			let watched = self.watched.lock().unwrap();
 			for (index, transaction) in block.txdata.iter().enumerate() {
 				if self.does_match_tx_unguarded(transaction, &watched) {
-					matched.push(transaction);
 					matched_index.push(index as u32);


Keep this as usize and return Vec<usize> since you are just casting it back at the call site.

Did so in a new commit since it had a bunch of followon effects.

jkczyz · 2020-06-12T23:41:37Z

lightning/src/ln/functional_tests.rs

@@ -257,7 +257,7 @@ fn test_update_fee_unordered_raa() {
 	// ...but before it's delivered, nodes[1] starts to send a payment back to nodes[0]...
 	let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
 	let net_graph_msg_handler = &nodes[1].net_graph_msg_handler;
-	nodes[1].node.send_payment(&get_route(&nodes[1].node.get_our_node_id(), net_graph_msg_handler, &nodes[0].node.get_our_node_id(), None, &Vec::new(), 40000, TEST_FINAL_CLTV, &logger).unwrap(), our_payment_hash, &None).unwrap();
+	nodes[1].node.send_payment(&get_route(&nodes[1].node.get_our_node_id(), &*net_graph_msg_handler.network_graph.read().unwrap(), &nodes[0].node.get_our_node_id(), None, &Vec::new(), 40000, TEST_FINAL_CLTV, &logger).unwrap(), our_payment_hash, &None).unwrap();


The dereference is unnecessary here and throughout.

The types are different - without the ref+deref its a MutexGuard, with the ref+deref you get the NetworkGraph itself.

You shouldn't need to explicitly dereference a MutexGuard. See how it is working correctly in fuzz/src/full_stack.rs without the dereference.

Actually, not sure if that is the same case, but it does compile here if you remove the dereference.

Ah, I'd tried previously without either the & or *, which obviously doesn't work, but you're right, works with only the &.

jkczyz · 2020-06-13T21:06:35Z

The changes related to get_funding_txo in 0acdc55 belong in 897a1ab.

Not sure if you saw my previous top-level comment (quoted here) but otherwise looks good.

TheBlueMatt · 2020-06-13T21:34:25Z

I did not. Good catch, fixed.

jkczyz · 2020-06-14T05:33:03Z

Looks like a few places in the fuzz code need to be updated for 020ce15.

valentinewallace

partial review

lightning/src/routing/network_graph.rs

valentinewallace

LGTM! Just some nits

valentinewallace · 2020-06-16T22:00:10Z

lightning/src/ln/channel.rs

@@ -3339,6 +3339,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 							channel_id: self.channel_id(),
 							data: "funding tx had wrong script/value".to_owned()
 						});
+					} else if txo_idx > 0xff_ff {


Should these checks be tested? or a TODO, or issue to test them?

Oh, oops, duh, funding_txo.index is a u16, so this line is unreachable.

... instead of only the txid. This is another instance of it not being possible to fully re-implement SimpleManyChannelMonitor using only public methods. In this case you couldn't properly register outpoints for monitoring so that the funding transaction would be matched.

In general, we don't need an explicit lifetime when doing something like: fn get_thing(&self) -> &Thing { &self.thing }. This also makes it easier to reason about what's going on in the bindings generation.

This isn't a big difference in the API, but it avoids needing to wrap a given NetworkGraph in a RwLock before passing it, which makes it much easier to generate C bindings for.

This is more consistent with the way we use std::cmp over the codebase and avoids `use std`, which is only actually needed to support older rustcs, so feels a bit awkward.

non-mut references to primitives are only excess overhead, so there's not much reason to ever have them. As a nice bonus, it also is one less thing to worry about when generating C bindings

Instead of making the filter_block fn in the ChainWatchInterface trait return both a list of indexes of transaction positions within the block and references to the transactions themselves, return only the list of indexes and then build the reference list at the callsite. While this may be slightly less effecient from a memory locality perspective, it shouldn't be materially different. This should make it more practical to generate bindings for filter_block as it no longer needs to reference Rust Transaction objects that are contained in a Rust Block object (which we'd otherwise just pass over the FFI in fully-serialized form).

This was just an oversight when route calculation was split up into parts - it makes no sense for get_route to require that we have a full route message handler, only a network graph (which can always be accessed from a NetGraphMsgHandler anyway).

We use them largely as indexes into a Vec<Transaction> so there's little reason for them to be u32s. Instead, use them as usize everywhere. We also take this opportunity to add range checks before short_channel_id calculation, as we could otherwise end up with a bogus short_channel_id due to an output index out of range.

TheBlueMatt · 2020-06-23T20:13:02Z

Squashed.

valentinewallace

Cool, updates LGTM

TheBlueMatt force-pushed the 2020-06-c-bindings-cleanups-2 branch 3 times, most recently from a85467f to b006acb Compare June 11, 2020 20:05

jkczyz self-requested a review June 12, 2020 18:16

jkczyz reviewed Jun 12, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-06-c-bindings-cleanups-2 branch from 912d57a to ef57682 Compare June 13, 2020 18:12

TheBlueMatt force-pushed the 2020-06-c-bindings-cleanups-2 branch from ef57682 to 020ce15 Compare June 13, 2020 21:34

TheBlueMatt force-pushed the 2020-06-c-bindings-cleanups-2 branch from 795ae9c to 7741f38 Compare June 16, 2020 19:56

valentinewallace reviewed Jun 16, 2020

View reviewed changes

lightning/src/routing/network_graph.rs Show resolved Hide resolved

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

valentinewallace approved these changes Jun 16, 2020

View reviewed changes

TheBlueMatt added 5 commits June 22, 2020 13:42

Drop some unnecessary lifetime specifiers in return type definitions

aa14fe5

In general, we don't need an explicit lifetime when doing something like: fn get_thing(&self) -> &Thing { &self.thing }. This also makes it easier to reason about what's going on in the bindings generation.

Drop a useless import of std::result::Result since its always there

9e58009

Reduce RwLock usage in public interface of NetworkGraph

c33a444

This isn't a big difference in the API, but it avoids needing to wrap a given NetworkGraph in a RwLock before passing it, which makes it much easier to generate C bindings for.

Avoid use std and use std::fmt and fmt:: instead in network_graph

922fc8e

This is more consistent with the way we use std::cmp over the codebase and avoids `use std`, which is only actually needed to support older rustcs, so feels a bit awkward.

TheBlueMatt force-pushed the 2020-06-c-bindings-cleanups-2 branch from 7741f38 to 8b4c5a3 Compare June 23, 2020 04:09

TheBlueMatt added 5 commits June 23, 2020 16:12

Avoid references to primitives and add NetworkGraph::new()

ee6dd7f

non-mut references to primitives are only excess overhead, so there's not much reason to ever have them. As a nice bonus, it also is one less thing to worry about when generating C bindings

Drop unused import in fuzz router.rs

7fc07b3

TheBlueMatt force-pushed the 2020-06-c-bindings-cleanups-2 branch from 8b4c5a3 to 5c37023 Compare June 23, 2020 20:13

valentinewallace approved these changes Jun 23, 2020

View reviewed changes

jkczyz approved these changes Jun 23, 2020

View reviewed changes

TheBlueMatt merged commit 8fae0c0 into lightningdevkit:master Jun 24, 2020

jkczyz mentioned this pull request Jun 25, 2020

Refactor chain monitoring #649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre-C bindings cleanups (2) #638

Pre-C bindings cleanups (2) #638

TheBlueMatt commented Jun 11, 2020

codecov bot commented Jun 11, 2020 •

edited

Loading

jkczyz left a comment

jkczyz Jun 12, 2020

TheBlueMatt Jun 13, 2020

jkczyz Jun 12, 2020

TheBlueMatt Jun 13, 2020

jkczyz Jun 12, 2020

TheBlueMatt Jun 13, 2020

jkczyz Jun 13, 2020

jkczyz Jun 13, 2020

TheBlueMatt Jun 13, 2020

jkczyz commented Jun 13, 2020

TheBlueMatt commented Jun 13, 2020

jkczyz commented Jun 14, 2020

valentinewallace left a comment •

edited

Loading

valentinewallace left a comment

valentinewallace Jun 16, 2020

TheBlueMatt Jun 23, 2020

TheBlueMatt commented Jun 23, 2020

valentinewallace left a comment

Pre-C bindings cleanups (2) #638

Pre-C bindings cleanups (2) #638

Conversation

TheBlueMatt commented Jun 11, 2020

codecov bot commented Jun 11, 2020 • edited Loading

Codecov Report

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Jun 13, 2020

TheBlueMatt commented Jun 13, 2020

jkczyz commented Jun 14, 2020

valentinewallace left a comment • edited Loading

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 23, 2020

valentinewallace left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 11, 2020 •

edited

Loading

valentinewallace left a comment •

edited

Loading