🤔 martinus requested changes to a pull request: "refactor: Use util::Result class for wallet loading"
(https://github.com/bitcoin/bitcoin/pull/25722#pullrequestreview-1386736519)
Code review, apart from the compile error when rebasing I found mostly nits. This makes `util::Result` quite a bit more powerful, and it's still simple to use. The code itself of util::Result is a bit hard to understand though, but I don't have a better way either.

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167765326)
Same here, I'd say `void MoveConstruct(Result<OT, OF>&& other)` is a bit clearer

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167765212)
Maybe it would be a bit clearer to use `(O&& other)` here?

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167765462)
Ha, nice trick; I've never seen such an inheritance before :-)

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167981120)
Mayby explicitly mark the move-assignment as delete
```cpp
template <typename OT, typename OF>
Result& operator=(Result<OT, OF>&& other) = delete;
```

Then the compiler error messages are a bit nicer when one tries to move-assign

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167821634)
It took me a while to figure out what's going on here with the recursive `Construct` calls and the lambda that's passed along. How about instead of passing a lambda along use a `bool` template argument like so:

```diff
diff --git a/src/util/result.h b/src/util/result.h
index 5599da43dd..3c163cf398 100644
--- a/src/util/result.h
+++ b/src/util/result.h
@@ -151,33 +151,35 @@ template <typename T, typename F = void>
class Result : public detail::ResultBase<T, F>
{
protected:
- te
...

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167981676)
nit: I don't think the `if (!src.m_info->errors.empty())` check is necessary.

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174297928)
maybe consider forbidding type `bool` for `T`. I recently had similar code in another project, and its utterly confusing; because Result can convert to bool but that only tells if it *has* a bool or not. It's very error prone.

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174298529)
I'd add a check somewhere to ensure that the size of the Result is as claimed in the commit message, something like that:

```cpp
static_assert(sizeof(util::Result<int>) == sizeof(void*)*2);
static_assert(sizeof(util::Result<void>) == sizeof(void*));
```

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174414223)
move constructors should usually be `noexcept`, but then all methods it uses also have to be `noexcept`.

💬 TheBlueMatt commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518672296)
BIP 322 is great, but isn't super practical to verify unless you have a full script interpreter (ie are Core). It also doesn't address the "this is an anti-feature" issue :).

Still, if that's added the existing message signing should be ripped out. Today it doesn't really have any use, and the intended uses people do have for it are not good outcomes.

💬 Mynima commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518678907)
> bitnorbert

à la Lightning channel updates.

🤔 ishaanam reviewed a pull request: "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1396747574)
Approach ACK

💬 ishaanam commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1174424310)
In 2b04a58972e18ffd06535ad2778547db7426b05d "[rpc] add getprioritisationmap"

nit: saying that this RPC "Returns a map of all fee deltas in memory pool" might be a bit confusing for users because you can run `prioritisetransaction` even if a tx is not yet in the mempool. Perhaps the following would be better:
```suggestion
"Returns a map of all user-created fee deltas by txid, and whether the tx is present in mempool.",

💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1174426912)
Done

💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1518682303)
Rebased to fix failing CI.

💬 jlopp commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518705299)
I'll note that message signing is an important feature we leverage for Casa clients to perform health checks on their keys and hardware devices without requiring them to actually sign a transaction. While at the software level, arbitrary signing of data with private keys could be accomplished pretty much however one wishes, it's important to us that a message signing standard exists so that there's a well defined spec for hardware device companies to support.

💬 theStack commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518711328)
> BIP 322 is great, but isn't super practical to verify unless you have a full script interpreter (ie are Core).

In theory, using [libconsensus](https://github.com/bitcoin/bitcoin/blob/master/doc/shared-libraries.md) for that shouldn't be too hard, especially if a proper language binding is available? (Though admittedly, it still lacks Taproot support -- https://github.com/bitcoin/bitcoin/pull/21158 is up for grabs).

💬 sipa commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518711858)
BIP322 doesn't actually need full script validation if partiality is acceptable (e.g. a verifier can just implement a specific templated subset of scripts, and reply inconclusive for anything beyond that). That may sound suboptimal, but a scheme that's restricted by design to some subset cannot do better either.

💬 mzumsande commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518718755)
some previous discussion in #24186.