Immutable BlockSource interface

[jkczyz]

Querying a BlockSource is a logically immutable operation. Use non-mut references in its interface to reflect this, which allows for users to hold multiple references if desired.

[TheBlueMatt]

Hmmm, &self in a trait is strictly less flexible than &mut self - you can always implement the trait as for &mut &Self, whereas you can't go the other way (&mut &Self is still a non-mutable reference to Self). Sure, the double-reference is less efficient, but its at least highly flexible. In this specific case, I could see a BlockSource being implemented over some kind of pipelined HTTP/etc client where you need exclusive access to an underlying socket to operate. You could "just add a lock" or whatever, but I'm not really entirely sure what the argument on the flip side is, here - when is &mut Self a real issue for block source implementors?

[jkczyz]

IIUC, the use case is the need for two BlockSource references in a multi-node setup. One to give to ChainPoller and another to synchronize_listeners. The first is used to keep all nodes in sync via an SpvClient. The second use is needed to have a paused node catch up before adding it back to the node set being kept in sync.

[TheBlueMatt]

Right, that specific use-case could be accomplished with an implementation for BlockSource on &Type, though.

[jkczyz]

I'm not sure how that's helpful. It won't let you mutate anything within impl BlockSource for &Type.

Also seems inconsistent with other traits like BroadcasterInterface and FeeEstimator that don't take &mut self.

[TheBlueMatt]

I'm not sure how that's helpful. It won't let you mutate anything within impl BlockSource for &Type.

Right, my point is for those who don't want to mutate and want multiple instances, you can do this trick. For those that do want to mutate, it being &mut self allows that.

Also seems inconsistent with other traits like BroadcasterInterface and FeeEstimator that don't take &mut self.

Yea, agreed we should be consistent, I'm just not sure what the right answer is here. &mut self is strictly more flexible, but as you point out is less "rust"-y and looks awkward at first glance.

[TheBlueMatt]

I take that back, after the discussion at https://app.slack.com/client/TT1FWCE01/C0177366AR2/thread/C0177366AR2-1648897983.912319 I feel like maybe we should go ahead with this. Can you rebase this @jkczyz?

[codecov-commenter]

Codecov Report

Merging #1307 (444d8fb) into main (0a0f87c) will decrease coverage by 0.32%.
The diff coverage is 85.36%.

❗ Current head 444d8fb differs from pull request most recent head 3cdbbf5. Consider uploading reports for the commit 3cdbbf5 to get more accurate results

@@            Coverage Diff             @@
##             main    #1307      +/-   ##
==========================================
- Coverage   90.83%   90.51%   -0.33%     
==========================================
  Files          73       71       -2     
  Lines       41266    39210    -2056     
  Branches    41266        0   -41266     
==========================================
- Hits        37486    35489    -1997     
+ Misses       3780     3721      -59     

Impacted Files	Coverage Δ
lightning-block-sync/src/lib.rs	93.78% <ø> (ø)
lightning-block-sync/src/rest.rs	65.45% <66.66%> (ø)
lightning-block-sync/src/rpc.rs	78.51% <75.00%> (ø)
lightning-block-sync/src/init.rs	93.56% <100.00%> (-2.25%)	⬇️
lightning-block-sync/src/poll.rs	91.66% <100.00%> (ø)
lightning-block-sync/src/test_utils.rs	93.87% <100.00%> (ø)
lightning-invoice/src/utils.rs	84.07% <0.00%> (-12.62%)	⬇️
lightning/src/ln/peer_handler.rs	48.41% <0.00%> (-4.21%)	⬇️
lightning/src/util/ser_macros.rs	86.89% <0.00%> (-2.25%)	⬇️
... and 36 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 0a0f87c...3cdbbf5. Read the comment docs.

[devrandom]

ACK

In general, I prefer to keep mut semantically obvious (i.e. if the trait function has a "read-like" verb, then self should be immutable). Otherwise, the caller has to unexpectedly worry about getting a mut reference, which feels like an abstraction break.

If the implementor then needs something mutable because of the specific implementation details, then interior mutability seems appropriate.

[TheBlueMatt]

Oops, this now conflicts with the changes to Cargo for 106.