💬 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.

💬 harding commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518723971)
> Signing arbitrary messages with Bitcoin private keys was always a bit strange. It was designed to handle only the narrow case of single-sig keys, and isn't well-defined for new address types. Worse, it doesn't allow for signing messages with more arbitrary scripts

LN requires channel owners sign their gossip messages in order to use the costliness of acquiring a UTXO to prevent DoS attacks. Both currently and in [proposed changes](https://lists.linuxfoundation.org/pipermail/lightning-dev/2
...

💬 dimitaracev commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#discussion_r1174466477)
I think `std::max(MinBIP9WarningHeight, nMinerConfirmationWindow)` is unnecessary since I doubt that `nMinerConfirmationWindow` would be higher than `MinBIP9WarningHeight`. As for the rest, it makes sense since it removes the burden of having to keep in mind that a parameter would need adjusting in the future. I initially changed it to a timestamp due to your comment on the other PR :smile:.

💬 yashviradia commented on issue "listtransactions results order unkown":
(https://github.com/bitcoin/bitcoin/issues/17739#issuecomment-1518889742)
Hi,
Is this issue still open? Then I would like to work on it.

📝 theStack opened a pull request: "test: simplify uint256 (de)serialization routines"
(https://github.com/bitcoin/bitcoin/pull/27516)
These routines look fancy, but do nothing more than converting between byte objects of length 32 to/from integers in little endian byte order and can be replaced by simple one-liners, using the `int.{from,to}_bytes` methods (available since Python 3.2).

💬 NicolasDorier commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518921583)
Just mentioning here that I removed the ability for message signing from NBitcoin a long time ago for same reasons.

People who really need it just have to code it themselves. (which is just a few lines you can get looking the history of NBitcoin's code)