Implement Block Tracker #21
Conversation
crates/btc-notify/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| #[derive(Clone)] |
crates/btc-notify/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| #[derive(Clone)] |
crates/btc-notify/src/lib.rs
Outdated
| impl BtcZmqConfig { | ||
| /// This generates a default config that will not connect to any of the bitcoind zeromq interfaces. It is useful in | ||
| /// conjunction with subsequent mutations for partial initialization. | ||
| pub fn empty() -> BtcZmqConfig { | ||
| BtcZmqConfig { | ||
| bury_depth: 6, | ||
| hashblock_connection_string: None, | ||
| hashtx_connection_string: None, | ||
| rawblock_connection_string: None, | ||
| rawtx_connection_string: None, | ||
| sequence_connection_string: None, | ||
| } | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubhashblock connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_hashblock_connection_string(mut self, s: &str) -> Self { | ||
| self.hashblock_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubhashtx connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_hashtx_connection_string(mut self, s: &str) -> Self { | ||
| self.hashtx_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubrawblock connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_rawblock_connection_string(mut self, s: &str) -> Self { | ||
| self.rawblock_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubrawtx connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_rawtx_connection_string(mut self, s: &str) -> Self { | ||
| self.rawtx_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubsequence connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_sequence_connection_string(mut self, s: &str) -> Self { | ||
| self.sequence_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the builder pattern. I like it. But it is missing the .build() method.
See https://rust-unofficial.github.io/patterns/patterns/creational/builder.html
There was a problem hiding this comment.
Why does it need one?
There was a problem hiding this comment.
Because then build() -> BtcZmq, no?
There was a problem hiding this comment.
This, as it stands right now, isn't exactly the builder pattern since there is no intermediate type to call .build upon to get the actual type that is being built. That said, you might want to check out bon to reduce some of the boilerplate involved.
The nicest way to do this is to use the typestate pattern to only expose accessors if the corresponding value has been set. But that is a massive overkill for this.
crates/btc-notify/src/lib.rs
Outdated
| pub struct Subscription<T> { | ||
| receiver: mpsc::Receiver<T>, | ||
| } | ||
|
|
||
| impl<T> Subscription<T> { | ||
| fn from_receiver(receiver: mpsc::Receiver<T>) -> Subscription<T> { | ||
| Subscription { | ||
| receiver, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<T> Stream for Subscription<T> { | ||
| type Item = T; | ||
|
|
||
| fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { | ||
| self.get_mut().receiver.poll_recv(cx) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need trait bounds (or trait markers) in T.
I'm not sure, but having a bound is always a good idea, if applicable.
There was a problem hiding this comment.
I don't know why we'd want to constrain it at all. The construction works independent of T
There was a problem hiding this comment.
having a bound is always a good idea, if applicable
Hard disagree. Code should be written in as general way as possible. The trait bound should only be invoked if the essential aspects of that trait are used. In this case, the subscription is a stream irrespective of the contents of the subscription. This is a somewhat trivial case since it's really just a trivial indirection from the underlying tokio channel but the principle still holds. Regardless of what you are subscribing to, the subscription behaves like a stream.
There was a problem hiding this comment.
Doesn't this T need to be Sync?
Yeah your argument makes sense. We can keep T as it is if it works fine
There was a problem hiding this comment.
Normally, I would be tempted to at least constrain the generic to Sized. But that's mostly because when using an unconstrained generic, I usually run into issues that is solved by constraining it with Sized. If that isn't the problem here, I'm fine with leaving it unconstrained.
There was a problem hiding this comment.
Yeah I can definitely see sized being something that kicks in as a requirement at some point so you may be right.
| use std::pin::Pin; | ||
| use std::sync::Arc; | ||
| use std::task::Context; | ||
| use std::task::Poll; |
There was a problem hiding this comment.
rustfmt should have grouped these more nicely, I think.
There was a problem hiding this comment.
I'll address before finality but I want to focus on things in the following order:
- Interface design
- Property testing completeness
- Implementation correctness (edge case handling)
- Algorithmic implementation efficiency
- Documentation clarity, correctness, and completeness
- Code organization (which files/order of presentation within files)
- Silicon implementation efficiency
I still consider this to be in stage 1. Feel free to leave feedback on any of the stages but I'll address in roughly this order. To the extent that you can focus review on the stage we're in (currently 1 and 2), that would be ideal. If something is preventing you from properly evaluating those two, let me know.
I'll add this stage checklist to the main body of the PR.
There was a problem hiding this comment.
That's fine. I only wanted to flag this because it's something that your editor should take care of for you instead of having to dedicate actual hours on this.
| /// BtcZmqConfig is the main configuration type used to establish the connection with the ZMQ interface of Bitcoin. It | ||
| /// accepts independent connection strings for each of the stream types. Any connection strings that are left as None | ||
| /// when initializing the BtcZmqClient will result in those streams going unmonitored. In the limit, this means that the | ||
| /// default BtcZmqConfig will result in a BtcZmqClient that does absolutely nothing (NOOP). | ||
| pub struct BtcZmqConfig { |
There was a problem hiding this comment.
We generally structure these docs to have a single header line followed by a longer paragraph. There is also a clippy lint for this that is allowed by default but something we might deny later. Tidying this up now will make our work much easier in the future.
The content itself is top-notch though!
There was a problem hiding this comment.
Yeah the content is REALLY good.
@ProofOfKeags check the engineering best practices for docstrings in this section of our notion doc: https://www.notion.so/Engineering-Rust-Best-Practices-14c901ba000f8025a974ec8e2a70b4af?pvs=4#14c901ba000f80b38a1fc290ef3a900a
crates/btc-notify/src/lib.rs
Outdated
| impl BtcZmqConfig { | ||
| /// This generates a default config that will not connect to any of the bitcoind zeromq interfaces. It is useful in | ||
| /// conjunction with subsequent mutations for partial initialization. | ||
| pub fn empty() -> BtcZmqConfig { | ||
| BtcZmqConfig { | ||
| bury_depth: 6, | ||
| hashblock_connection_string: None, | ||
| hashtx_connection_string: None, | ||
| rawblock_connection_string: None, | ||
| rawtx_connection_string: None, | ||
| sequence_connection_string: None, | ||
| } | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubhashblock connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_hashblock_connection_string(mut self, s: &str) -> Self { | ||
| self.hashblock_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubhashtx connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_hashtx_connection_string(mut self, s: &str) -> Self { | ||
| self.hashtx_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubrawblock connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_rawblock_connection_string(mut self, s: &str) -> Self { | ||
| self.rawblock_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubrawtx connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_rawtx_connection_string(mut self, s: &str) -> Self { | ||
| self.rawtx_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
|
|
||
| /// Updates the BtcZmqConfig with a zmqpubsequence connection string and returns the updated config. Useful for a | ||
| /// builder pattern with dotchaining. | ||
| pub fn with_sequence_connection_string(mut self, s: &str) -> Self { | ||
| self.sequence_connection_string = Some(s.to_string()); | ||
| self | ||
| } | ||
| } |
There was a problem hiding this comment.
This, as it stands right now, isn't exactly the builder pattern since there is no intermediate type to call .build upon to get the actual type that is being built. That said, you might want to check out bon to reduce some of the boilerplate involved.
The nicest way to do this is to use the typestate pattern to only expose accessors if the corresponding value has been set. But that is a massive overkill for this.
crates/btc-notify/src/lib.rs
Outdated
| pub struct Subscription<T> { | ||
| receiver: mpsc::Receiver<T>, | ||
| } | ||
|
|
||
| impl<T> Subscription<T> { | ||
| fn from_receiver(receiver: mpsc::Receiver<T>) -> Subscription<T> { | ||
| Subscription { | ||
| receiver, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<T> Stream for Subscription<T> { | ||
| type Item = T; | ||
|
|
||
| fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { | ||
| self.get_mut().receiver.poll_recv(cx) | ||
| } | ||
| } |
There was a problem hiding this comment.
Normally, I would be tempted to at least constrain the generic to Sized. But that's mostly because when using an unconstrained generic, I usually run into issues that is solved by constraining it with Sized. If that isn't the problem here, I'm fine with leaving it unconstrained.
crates/btc-notify/src/lib.rs
Outdated
|
|
||
| fn setup() -> Result<(BtcZmqClient, corepc_node::Node), Box<dyn std::error::Error>> { | ||
| let mut bitcoin_conf = corepc_node::Conf::default(); | ||
| bitcoin_conf.view_stdout = true; |
There was a problem hiding this comment.
You should be able to get away with just setting RUST_LOG to trace for debugging purposes. Is there any reason we need the stdout too though?
There was a problem hiding this comment.
This was for my own ability to debug stuff since I wanted to ensure that the bitcoind side was actually publishing on the zmq interface.
Can you clarify whatyou mean by:
You should be able to get away with just setting RUST_LOG to trace for debugging purposes.
There was a problem hiding this comment.
You can set the environment variable RUST_LOG to trace and that should produce a lot more logs from corepc-node including the actual requests and response. This requires that you first initialize the logger which you can do via strata_common::logging::init(...). More on this here: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html
| let newly_mined = bitcoind.client.generate_to_address(1, &bitcoind.client.new_address()?)?.into_model()?; | ||
| let blk = block_sub.next().await.map(|b|b.block_hash()); | ||
| assert_eq!(newly_mined.0.first(), blk.as_ref()); | ||
| drop(client); |
There was a problem hiding this comment.
Is this explicit drop necessary? If we are doing this, should we also check if the thread has indeed been aborted?
There was a problem hiding this comment.
This is to ensure that rustc does not get cute with doing a premature drop, causing the producer thread to terminate prematurely (by virtue of its drop trait)
There was a problem hiding this comment.
should we also check if the thread has indeed been aborted?
That's a great property to test. I'd have to think about how to design such a test though.
There was a problem hiding this comment.
The simplest way to change the visible of the JoinHandle to pub(crate) and then, check for is_canceled. But that may be too literal/specific to qualify as a good test.
There was a problem hiding this comment.
I think the other test design here would be to ensure that all subscription channels have their send sides closed when the client is dropped. Because the subscription doesn't contain references to the client, it isn't automatically invalidated by lifetime analysis (which is probably a good thing). I'm hesitant to leak the JoinHandle since it breaks some of the abstraction. I'm focusing on tests right now so I'll probably have better ideas in the next day or so here.
| self.tx_lifecycles.insert(matched_tx.compute_txid(), lifecycle); | ||
| } | ||
| Some(lifecycle) => { | ||
| match (&lifecycle.raw, lifecycle.block, lifecycle.seq_received) { |
There was a problem hiding this comment.
Looks like there are a bunch of runtime invariants that we are trying to handle. The comments definitely help but is there a way to encode this at compile time instead? Perhaps with enums with struct variants?
There was a problem hiding this comment.
Completely agree with this push. The answer is I believe that we can. As I was going through implementation I think I figured out a way to do a CBC encoding of this to reduce and possibly eliminate the runtime invariants, but I wanted to complete a full sketch before going back and refining.
| pub async fn subscribe_transactions(&mut self, f: impl Fn(&Transaction) -> bool + Sync + Send + 'static) -> | ||
| Subscription<(Transaction, TxStatus)> { | ||
|
|
||
| let (send, recv) = mpsc::channel(4); |
There was a problem hiding this comment.
Is there any specific reason to go with this queue size?. Same question for the choice of 10 in the subscribe_blocks method below.
There was a problem hiding this comment.
No they were just best guesses that I went with to start. I'll drop a TODO in there to give a more careful thought to the magick numbers.
ProofOfKeags
force-pushed
the
STR-821-duty-tracker-interface
branch
from
d3a76dc to
0083e8c
Compare
Description
This PR implements a reorg resistant blockchain tracker that should correctly track the full lifecycle of any transactions that a consumer wants to monitor. The PR should be reviewed in stages, giving attention to different aspects during progression. This should help reduce the amount of unnecessary review cycles, allowing us to solve the bigger, more foundational problems first, and move onto refinement after.
Type of Change
Checklist
Related Issues