Doc and build warning fixes

[jkczyz]

No description provided.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.61 🎉

Comparison is base (f114515) 87.15% compared to head (1d1323a) 87.77%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2068      +/-   ##
==========================================
+ Coverage   87.15%   87.77%   +0.61%     
==========================================
  Files         100      100              
  Lines       44519    49520    +5001     
  Branches    44519    49520    +5001     
==========================================
+ Hits        38799    43464    +4665     
- Misses       5720     6056     +336     

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	81.88% <ø> (ø)
lightning-block-sync/src/lib.rs	87.31% <ø> (ø)
lightning-block-sync/src/rpc.rs	84.48% <ø> (-0.14%)	⬇️
lightning-rapid-gossip-sync/src/processing.rs	91.70% <ø> (ø)
lightning/src/sync/fairrwlock.rs	75.00% <ø> (ø)
lightning/src/sync/test_lockorder_checks.rs	100.00% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.22% <100.00%> (+2.63%)	⬆️
lightning/src/ln/outbound_payment.rs	80.60% <100.00%> (ø)
lightning/src/sync/debug_sync.rs	82.53% <100.00%> (+1.32%)	⬆️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

This doesnt seem super helpful, it doesn't mention that the block source may well be returning an RpcError, which I think is the intent behind it. More generally why "may", isn't it does? Maybe we just leave this out and fix the error handling in a followup to just have enums and return proper errors rather than making users downcast several times.

[jkczyz]

RpcError is specific to RpcClient. Other BlockSources don't use it.

[TheBlueMatt]

Right, but that isn't documented anywhere in RpcClient, which seemed like part of the reason for this.

[jkczyz]

It's on RpcClient::call_method. Let me put it up on the struct docs, too, and explicitly tie it to BlockSource.

[TheBlueMatt]

Thanks for fixing the warnings, would be nice to land for 114.

[wpaulino]

There are a few warnings left on the no-std release build, not sure if we really care about those though.

[TheBlueMatt]

Ideally we'd fix them, yea, but none of its a huge deal.

[jkczyz]

There are a few warnings left on the no-std release build, not sure if we really care about those though.

Ah, will do. I just missed lightning-background-processor.

[jkczyz]

Ah, will do. I just missed lightning-background-processor.

Many of these are because lightning-background-processor doesn't have a no-std feature. Rather it can have either std, futures, or both. Should we just enforce at least one is set? Or will that break things? Seems littering the file with cfgs isn't ideal.

[TheBlueMatt]

Many of these are because lightning-background-processor doesn't have a no-std feature. Rather it can have either std, futures, or both. Should we just enforce at least one is set? Or will that break things? Seems littering the file with cfgs isn't ideal.

The whole "you must set either no-std or std" thing is really bad rust, its just that cargo's feature flags suck. If we can avoid it, we absolutely should - the "base crate" supports nothing, and then you can optionally turn on std or futures support, but they're separate concepts. There's nothing to "turn on" for no-std, so it shouldn't have a feature flag.

[jkczyz]

The whole "you must set either no-std or std" thing is really bad rust, its just that cargo's feature flags suck. If we can avoid it, we absolutely should - the "base crate" supports nothing, and then you can optionally turn on std or futures support, but they're separate concepts. There's nothing to "turn on" for no-std, so it shouldn't have a feature flag.

Makes sense. We probably want separate files for std, futures, and both, I'd imagine. And then just condition the pub use? Not sure if it is worth doing now, but I've at least fixed the ones when using --no-default-features --features=futures.

[TheBlueMatt]

LGTM, feel free to squash.

[wpaulino]

LGTM, but we should wait until we're done merging everything so we can rebase and make sure no more warnings slip through.