Split out BroadcastInterface, ChainWatchInterface monitors re-enter f… #11

tamasblummer · 2018-03-13T17:56:49Z

Removed dependency on bitcoin-chain (removed useless rustbitcoinchain.rs)
Split out BroadcastInterface from ChainWatchInterface as it will be likely implemented by a different component. Its use is also not linked with watching.
ChainWatchInterfaceUtil now watches for re-entry from called listener
@TheBlueMatt

…rom called listeners

TheBlueMatt

Few API comments that need a response, but generally seems fine-enough.

TheBlueMatt · 2018-03-15T02:36:29Z

src/chain/bitcoincorerpcchain.rs

 impl BitcoinCoreRPCClientChain {
 	pub fn new() -> BitcoinCoreRPCClientChain {
 		BitcoinCoreRPCClientChain {
 			util: ChainWatchInterfaceUtil::new(),
 		}
 	}
+
+	pub fn block_connected(&self, block: &Block, height: u32) {


I kinda envisioned this would have some polling thread (or a function to hit to make it poll) pinging the RPC and then generate the events there, not expose a pub fn which lets users call block_connected?

In case of SPV this is not polling, but an incoming headers message processed and will call this.

TheBlueMatt · 2018-03-15T02:41:23Z

src/chain/chaininterface.rs

+/// that peers rejected the transaction.
+pub trait BroadcasterInterface: Sync + Send {
+	/// Sends a transaction out to (hopefully) be mined
+	fn broadcast_transaction(&self, tx: &Transaction) -> Result<(), Box<Error>>;


I'm not sure if its useful to return a Result here - Bitcoin transaction broadcasting is always somewhat a "fire and see if anything happens" process. In other words, there is always a high chance of undetectable failure, so trying to return failure is a bit useless, or to put it another way, is there any useful handling you can do except to try again later (which should happen anyway?). Maybe I'm just being idealistic about it?

There are immediate error conditions that would better be communicated back, such as no connection to any peer or all peers rejected the message. An Ok result surely does not mean any confirmation, but without it does not even make sense to wait if anything happens as it won't.

TheBlueMatt · 2018-03-15T02:41:51Z

src/chain/chaininterface.rs

@@ -54,44 +60,79 @@ pub enum ConfirmationTarget {
 /// called from inside the library in response to ChainListener events, P2P events, or timer
 /// events).
 pub trait FeeEstimator: Sync + Send {
-	fn get_est_sat_per_vbyte(&self, ConfirmationTarget) -> u64;
+	fn get_est_sat_per_vbyte(&self, confirmation_target: ConfirmationTarget) -> u64;


nit: Does it matter?

compiler complained that anonymous arguments are deprecated.

TheBlueMatt · 2018-03-15T02:42:49Z

src/ln/channelmonitor.rs

 		let res = Arc::new(SimpleManyChannelMonitor {
 			monitors: Mutex::new(HashMap::new()),
-			chain_monitor: chain_monitor,
+			chain_monitor,


Oh, thats cool...learn a new syntax every day, I guess :p.

TheBlueMatt · 2018-03-15T02:45:43Z

src/chain/chaininterface.rs

 	}

-	pub fn watch_all_txn(&self) { //TODO: refcnt this?


I believe this TODO was intended to point out that we may want to refcount the watch_all, and have something like an unwatch_all, but then you'd need to have a count of how many times it was called? Anyway, probably worth getting rid of...

there is also no unregister. Let see if it is sufficient.

TheBlueMatt · 2018-03-15T02:48:42Z

src/chain/chaininterface.rs

+	/// notify listener that a block was connected
+	/// notification will repeat if notified listener register new listeners
+	pub fn block_connected(&self, block: &Block, height: u32) {
+		let mut watch = self.reentered.load(Ordering::Relaxed);


Is relaxed actually OK here? If you have multiple threads calling fetch_add(1) are you allowed to see reentered go backwards? I assume not, but I dont think the performance hit of something stricter matters?

enters are not strictly counted, the requirement is only to notice that there was a re-entry.

TheBlueMatt · 2018-03-15T02:50:07Z

src/chain/chaininterface.rs

+			let mut matched = Vec::new();
+			let mut matched_index = Vec::new();
+			for (index, transaction) in block.txdata.iter().enumerate() {
+				if self.does_match_tx(transaction) {


Hmm, you're taking a lock inside of the tight loop here, maybe worth pulling the code into this function and out of the helper so that you dont do so?

yes it would make sense to pull in the helper code. I did not here to make review easier not changing helper if not strictly needed.

TheBlueMatt · 2018-03-15T02:51:05Z

src/chain/chaininterface.rs

@@ -113,7 +155,7 @@ impl ChainWatchInterfaceUtil {
 	}

 	/// Checks if a given transaction matches the current filter
-	pub fn does_match_tx(&self, tx: &Transaction) -> bool {
+	fn does_match_tx(&self, tx: &Transaction) -> bool {


Why skip this from being pub? If people use some compression-like block downloads (BIP 37, the neutrino stuff, some PIR scheme, whatever) then its probably still needed?

yes, you are right. I make it pub again

TheBlueMatt · 2018-03-15T02:51:25Z

src/chain/chaininterface.rs

@@ -102,7 +143,8 @@ impl ChainWatchInterfaceUtil {
 		}
 	}

-	pub fn do_call_block_disconnected(&self, header: &BlockHeader) {
+	/// call listeners for disconnected blocks if they are still around
+	fn do_call_block_disconnected(&self, header: &BlockHeader) {


Why make this non-pub?

block_connected and block_disconnected meant to be called not these. I would probably inline them into those if I were not trying to make review easier.

Maybe just delete this function and move it into block_disconnected instead of having the stub?

TheBlueMatt · 2018-03-18T22:21:39Z

src/chain/bitcoincorerpcchain.rs

 	fn register_listener(&self, listener: Weak<ChainListener>) {
 		self.util.register_listener(listener)
 	}
 }

+impl BroadcasterInterface for BitcoinCoreRPCClientChain {
+	fn broadcast_transaction(&self, tx: &Transaction) {


New unused warning - can you use _tx?

TheBlueMatt · 2018-03-18T22:31:50Z

src/chain/chaininterface.rs

+		self.do_call_block_disconnected(header);
+	}
+
+	/// call listeners for connected blocks if they are still around. public only because of tests


I think this should be public generally, and should probably do the reentry check inside of itself here. ie move the let mut watch = self.reentered.load... to above the calls, and then load it again at the end, returning true/false if the user needs to call it again. That will simplify block_connected a bit, and also enable people who havent done the full-Block download to use the API easier. Then maybe rename it from do_call to block_connected_checked and block_connected something like block_connected_with_filtering.

TheBlueMatt · 2018-03-18T22:32:58Z

src/chain/chaininterface.rs

@@ -102,7 +143,8 @@ impl ChainWatchInterfaceUtil {
 		}
 	}

-	pub fn do_call_block_disconnected(&self, header: &BlockHeader) {
+	/// call listeners for disconnected blocks if they are still around
+	fn do_call_block_disconnected(&self, header: &BlockHeader) {


Maybe just delete this function and move it into block_disconnected instead of having the stub?

TheBlueMatt

Also a few new unused import warnings.

TheBlueMatt · 2018-03-18T22:35:48Z

src/chain/bitcoincorerpcchain.rs

 use bitcoin::blockdata::transaction::Transaction;
 use bitcoin::blockdata::script::Script;
+use bitcoin::blockdata::block::{Block, BlockHeader};


Unused import.

TheBlueMatt · 2018-03-18T22:35:52Z

src/chain/bitcoincorerpcchain.rs

@@ -1,8 +1,10 @@
+use std::error::Error;


Unused import.

TheBlueMatt · 2018-03-18T22:36:07Z

src/chain/chaininterface.rs

@@ -1,9 +1,10 @@
-use bitcoin::blockdata::block::BlockHeader;
+use std::error::Error;


Unused import.

renamed do_call_block_connected to bloc_connected_checked inlined do_block_disconnected. removed unused imports.

tamasblummer · 2018-03-19T07:40:00Z

@TheBlueMatt made the requested changes plus ignored IntelliJ files in .gitingnore

TheBlueMatt

Thanks.

TheBlueMatt · 2018-03-19T21:36:35Z

Squashed + Merged.

# This is the 1st commit message: test utils: refactor fail_payment_along_route for mpp # This is the commit message #2: Add MppId field to HTLCSource as a way to correlate mpp payment paths # This is the commit message #3: Implement Readable/Writeable for HashSet To be used in upcoming commits for MPP ID storage # This is the commit message #4: Add MPP ID to pending_outbound_htlcs We'll use this to correlate MPP shards in upcoming commits # This is the commit message #5: Prevent duplicate PaymentSent events by removing all pending outbound payments associated with the same MPP payment after the preimage is received # This is the commit message #6: Add all_paths_failed field to PaymentFailed see field docs for details # This is the commit message #7: Drop writer size hinting/message vec preallocation In order to avoid significant malloc traffic, messages previously explicitly stated their serialized length allowing for Vec preallocation during the message serialization pipeline. This added some amount of complexity in the serialization code, but did avoid some realloc() calls. Instead, here, we drop all the complexity in favor of a fixed 2KiB buffer for all message serialization. This should not only be simpler with a similar reduction in realloc() traffic, but also may reduce heap fragmentation by allocating identically-sized buffers more often. # This is the commit message #8: [fuzz] Swap mode on most messages to account for TLV suffix # This is the commit message #9: Add forward-compat due serialization variants of HTLCFailureMsg Going forward, all lightning messages have a TLV stream suffix, allowing new fields to be added as needed. In the P2P protocol, messages have an explicit length, so there is no implied length in the TLV stream itself. HTLCFailureMsg enum variants have messages in them, but without a size prefix or any explicit end. Thus, if a HTLCFailureMsg is read as a part of a ChannelManager, with a TLV stream at the end, there is no way to differentiate between the end of the message and the next field(s) in the ChannelManager. Here we add two new variant values for HTLCFailureMsg variants in the read path, allowing us to switch to the new values if/when we add new TLV fields in UpdateFailHTLC or UpdateFailMalformedHTLC so that older versions can still read the new TLV fields. # This is the commit message #10: Convert most P2P msg serialization to a new macro with TLV suffixes The network serialization format for all messages was changed some time ago to include a TLV suffix for all messages, however we never bothered to implement it as there isn't a lot of use validating a TLV stream with nothing to do with it. However, messages are increasingly utilizing the TLV suffix feature, and there are some compatibility concerns with messages written as a part of other structs having their format changed (see previous commit). Thus, here we go ahead and convert most message serialization to a new macro which includes a TLV suffix after a series of fields, simplifying several serialization implementations in the process. # This is the commit message #11: Update docs to specify where process events is called

# This is the 1st commit message: Update fuzz README with latest instructions # This is the commit message #2: Allow multiple monitor_update_failed calls without requiring calls to channel_monitor_updated in between. Found by the fuzzer # This is the commit message #3: Move CounterpartyForwardingInfo from channel to channelmanager CounterpartyForwardingInfo is public (previously exposed with a `pub use`), and used inside of ChannelCounterparty in channelmanager.rs. However, it is defined in channel.rs, away from where it is used. This would be fine, except that the bindings generator is somewhat confused by this - it doesn't currently support interpreting `pub use` as a struct to expose, instead ignoring it. Fixes lightningdevkit/ldk-garbagecollected#44 # This is the commit message #4: test utils: refactor fail_payment_along_route for mpp # This is the commit message #5: Add MppId field to HTLCSource as a way to correlate mpp payment paths # This is the commit message #6: Implement Readable/Writeable for HashSet To be used in upcoming commits for MPP ID storage # This is the commit message #7: Add MPP ID to pending_outbound_htlcs We'll use this to correlate MPP shards in upcoming commits # This is the commit message #8: Prevent duplicate PaymentSent events by removing all pending outbound payments associated with the same MPP payment after the preimage is received # This is the commit message #9: Add all_paths_failed field to PaymentFailed see field docs for details # This is the commit message #10: Drop writer size hinting/message vec preallocation In order to avoid significant malloc traffic, messages previously explicitly stated their serialized length allowing for Vec preallocation during the message serialization pipeline. This added some amount of complexity in the serialization code, but did avoid some realloc() calls. Instead, here, we drop all the complexity in favor of a fixed 2KiB buffer for all message serialization. This should not only be simpler with a similar reduction in realloc() traffic, but also may reduce heap fragmentation by allocating identically-sized buffers more often. # This is the commit message #11: [fuzz] Swap mode on most messages to account for TLV suffix # This is the commit message #12: Add forward-compat due serialization variants of HTLCFailureMsg Going forward, all lightning messages have a TLV stream suffix, allowing new fields to be added as needed. In the P2P protocol, messages have an explicit length, so there is no implied length in the TLV stream itself. HTLCFailureMsg enum variants have messages in them, but without a size prefix or any explicit end. Thus, if a HTLCFailureMsg is read as a part of a ChannelManager, with a TLV stream at the end, there is no way to differentiate between the end of the message and the next field(s) in the ChannelManager. Here we add two new variant values for HTLCFailureMsg variants in the read path, allowing us to switch to the new values if/when we add new TLV fields in UpdateFailHTLC or UpdateFailMalformedHTLC so that older versions can still read the new TLV fields. # This is the commit message #13: Convert most P2P msg serialization to a new macro with TLV suffixes The network serialization format for all messages was changed some time ago to include a TLV suffix for all messages, however we never bothered to implement it as there isn't a lot of use validating a TLV stream with nothing to do with it. However, messages are increasingly utilizing the TLV suffix feature, and there are some compatibility concerns with messages written as a part of other structs having their format changed (see previous commit). Thus, here we go ahead and convert most message serialization to a new macro which includes a TLV suffix after a series of fields, simplifying several serialization implementations in the process. # This is the commit message #14: Update docs to specify where process events is called

Indicate whether a channel is public in `listchannels`

Split out BroadcastInterface, ChainWatchInterface monitors re-enter f…

c58e2d6

…rom called listeners

tamasblummer force-pushed the chaininterface branch from 4569d1c to c58e2d6 Compare March 13, 2018 20:22

fixing tests

8dde2f6

TheBlueMatt requested changes Mar 15, 2018

View reviewed changes

tamasblummer added 4 commits March 15, 2018 10:36

match_tx public again, lock on watched is no longer in a tight loop

505c1f1

do not assume how RPC client will use the util class

db224f7

broadcast does not return an error.

abda523

no error return from bradcast

e820c1c

TheBlueMatt requested changes Mar 18, 2018

View reviewed changes

TheBlueMatt reviewed Mar 18, 2018

View reviewed changes

tamasblummer added 2 commits March 19, 2018 08:38

renamed block_connected to block_connected_with_filtering.

15165f1

renamed do_call_block_connected to bloc_connected_checked inlined do_block_disconnected. removed unused imports.

ignore IntelliJ IDEA files

3e0c8a4

TheBlueMatt approved these changes Mar 19, 2018

View reviewed changes

TheBlueMatt added a commit that referenced this pull request Mar 19, 2018

Merge pull request #11 from tamasblummer/chaininterface (squashed)

ad9dda2

TheBlueMatt closed this Mar 19, 2018

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Mar 19, 2018

Clean up a few comments after lightningdevkit#11 merge.

a5ec986

TheBlueMatt mentioned this pull request Mar 19, 2018

Few random fixes #12

Merged

tnull mentioned this pull request Jun 10, 2022

Document better optional features #1514

Merged

carlaKC referenced this pull request in carlaKC/rust-lightning Aug 9, 2023

Merge pull request #11 from valentinewallace/expose-channel-publicness

c3218f2

Indicate whether a channel is public in `listchannels`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split out BroadcastInterface, ChainWatchInterface monitors re-enter f… #11

Split out BroadcastInterface, ChainWatchInterface monitors re-enter f… #11

tamasblummer commented Mar 13, 2018

TheBlueMatt left a comment

TheBlueMatt Mar 15, 2018

tamasblummer Mar 15, 2018

TheBlueMatt Mar 15, 2018

tamasblummer Mar 15, 2018

TheBlueMatt Mar 15, 2018

tamasblummer Mar 15, 2018

TheBlueMatt Mar 15, 2018

TheBlueMatt Mar 15, 2018

tamasblummer Mar 15, 2018

TheBlueMatt Mar 15, 2018

tamasblummer Mar 15, 2018

TheBlueMatt Mar 15, 2018

tamasblummer Mar 15, 2018

TheBlueMatt Mar 15, 2018

tamasblummer Mar 15, 2018

TheBlueMatt Mar 15, 2018

tamasblummer Mar 15, 2018

TheBlueMatt Mar 18, 2018

TheBlueMatt Mar 18, 2018

TheBlueMatt Mar 18, 2018

TheBlueMatt Mar 18, 2018

TheBlueMatt left a comment

TheBlueMatt Mar 18, 2018

TheBlueMatt Mar 18, 2018

TheBlueMatt Mar 18, 2018

tamasblummer commented Mar 19, 2018

TheBlueMatt left a comment

TheBlueMatt commented Mar 19, 2018

		@@ -1,9 +1,10 @@
		use bitcoin::blockdata::block::BlockHeader;
		use std::error::Error;

Split out BroadcastInterface, ChainWatchInterface monitors re-enter f… #11

Split out BroadcastInterface, ChainWatchInterface monitors re-enter f… #11

Conversation

tamasblummer commented Mar 13, 2018

TheBlueMatt 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt 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

tamasblummer commented Mar 19, 2018

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 19, 2018