💬 GregTonoski commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518560156)
> Sadly, the message signing feature is now being abused to "verify wallets" as a part of withdraw flows

Abusers may resolve the issue themselves and there isn't any Bitcoin Core change that could stop them. I suggest contacting abusers instead of Bitcoin Core community.

Besides, users like the message signing feature. Not only in Bitcoin Core. There are many wallets that implement it.

💬 furszy 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_r1174395264)
Yeah, nothing biggie, just:
```c++
auto try_updating_state = [disconnect_height](CWalletTx& tx) {
```
so only the required elements are provided to the lambda.

💬 furszy 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_r1174393895)
Can remove the context ref.
```suggestion
auto try_updating_state = [](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
```

💬 amtriorix commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518646177)
Are you nuts or what?!

💬 aceofbitcoin commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1518653817)
You can indeed use your private keys of bitcoin to digitally sign messages. If You don't understand the basics of assymetric keys used to sign/verify transactions, messages or any text, You should first read more books about pgp, bitcoin and assymmetric encryption. I really don't like such noob proposals. #satoshin

💬 Tracachang commented on issue "Export a watch wallet only (with descriptors and without private keys) for an air gap setup":
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1518657586)
> > Just tried again following this commands and I am still unable to see taproot in GUI
>
> at all? or just in the watch-only wallet?

Just on GUI with the watch-only wallet, but on console using watch-only I can create taproot receiving addresses.

💬 Tracachang commented on issue "Write instructions on offline signing.":
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1518659800)
Hello, I've ever wrote a step by step tutorial on bitcointalk forum about offline signing if it can help: [https://bitcointalk.org/index.php?topic=5392824.msg59740404#msg59740404](url)

It is one year old but it is up to date, screenshots are not online any more but I do still have them on a i2p website if needed.

💬 martinus commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#issuecomment-1518662504)
After rebasing I get a compile error in `ChooseSelectionResult`, adding a bunch of `std::move` helps

💬 mzumsande commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1518666764)
I think that in the suggested approach we might reveal information by _not_ inv'ing our own tx to others.
If a spy is connected to each node, we might send the tx to them first currently (master), but with the PR, if we are now the only node that never inv's the tx to them at all (provided that all other nodes do that eventually with the flooding mechanism) that would equally identify us as the source.

🤔 theStack reviewed a pull request: "BIP324: ElligatorSwift integrations"
(https://github.com/bitcoin/bitcoin/pull/27479#pullrequestreview-1396744957)
Code-review ACK 818acac23295d526d9f85f0218949c817f5df964 (starting from commit fd04a2baec77b97cfb36ec488cec58811ea1ad45; didn't review any of the secp256k1 changes introduced in https://github.com/bitcoin-core/secp256k1/pull/1129)

🤔 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`.