💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1109676251)
Not sure. I think I'd rather have an error message for mixed use of `h` and `'`.

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#issuecomment-1434522252)
Rebased, added the suggested fuzzer coverage and fixed some `'` that I missed.

> What was the rationale for only normalizing the public-key version with h?

I was thinking along the lines that normalization (moving the xpub to a logical position, converting `'` -> `h`) was an opt-in thing that we only do with the public-key version. However it's pretty much the default in practice, e.g. `listdescriptors` normalizes. We might as well do it for the private key version. The private key versio
...

📝 darosior opened a pull request: "fuzz: avoid redundant dup key checks when creating Miniscript nodes"
(https://github.com/bitcoin/bitcoin/pull/27117)
I thought i had done that already in #24149, but it must have slipped through the rebase. It's a 2x speed improvement against the existing corpora and will probably be much more as we extend them with larger nodes.

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#issuecomment-1434523714)
Oops, didn't rebase far enough to get past the merge conflict...

💬 darosior commented on pull request "fuzz: avoid redundant dup key checks when creating Miniscript nodes":
(https://github.com/bitcoin/bitcoin/pull/27117#issuecomment-1434523841)
cc @sipa

💬 darosior commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1109682908)
https://github.com/bitcoin/bitcoin/pull/27117

💬 hebasto commented on pull request "build: Propagate user-defined tools to native packages":
(https://github.com/bitcoin/bitcoin/pull/23571#issuecomment-1434524289)
Closing. Up for grabs.

✅ hebasto closed a pull request: "build: Propagate user-defined tools to native packages"
(https://github.com/bitcoin/bitcoin/pull/23571)

💬 ajtowns commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#issuecomment-1434539481)
These aren't precisely the same (maybe they should be?). With `AssertLockNotHeld` you just get a report on that lock, on stderr:

```
Assertion failed: lock flock held in net.cpp:1398; locks held:
'flock' in net.cpp:1404 (in thread 'dnsseed')
```

with a bare `LOCK()` you get a less useful report on stderr (it only shows the location of the second lock attempt, not where it's being held):

```
Assertion failed: detected double lock for 'flock' in net.cpp:1399 (in thread 'dnsseed'), det
...

💬 Sjors commented on pull request "contrib: remove install_db4.sh":
(https://github.com/bitcoin/bitcoin/pull/26834#issuecomment-1434541595)
Post merge: just noticed the script was gone, but the bdb-only-depends approach works.

📝 fanquake opened a pull request: "depends: harden libevent"
(https://github.com/bitcoin/bitcoin/pull/27118)
Turn on libevents own hardening option (`--enable-gcc-hardneing`, although the options are not GCC specific), and backport a commit I upstreamed (https://github.com/libevent/libevent/pull/1418) to use `FORTIFY_SOURCE=3` over 2.

Solves half of #27038, by giving us some fortified funcs in `bitcoin-cli`.
Also elated to #27027.

🤔 vasild requested changes to a pull request: "p2p: return `CSubNet` in `LookupSubNet`"
(https://github.com/bitcoin/bitcoin/pull/26078)

💬 vasild commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1109707211)
nit: maybe add `assert(!subnet.IsValid());` after `CSubNet subnet;` to make it obvious that we return an invalid object and to guard against future changes that construct a valid `CSubNet` by default.

💬 vasild commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1109701750)
nit, here and elsewhere:

```suggestion
const CSubNet subnet = LookupSubNet(strAllow);
```

💬 vasild commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1109711519)
Hmm, `possibleSubnet.has_value()` will always be `true`. I guess this is an artifact from a previous incarnation of this PR that used `std::optional`.

💬 vasild commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1109752206)
Now `ResolveSubNet()` is identical to `LookupSubNet()`. Maybe ditch the former and change the callers to use directly `LookupSubNet()`?

💬 vasild commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1109749273)
`x || !x` will never be `false`. It suffices to change this to `(void)LookupSubNet(name);`.

💬 llazzaro commented on pull request "lint: enable E722 do not use bare except":
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1434618912)
Thanks for reviewing my code. I've identified a bare except statement that could catch exceptions it shouldn't. To improve code quality and precision, I've enabled the E722 flag.

This will help make the code more maintainable and avoid potential issues. I'm committed to following best practices and making more contributions to improve the project. Let me know if you have any further concerns.

💬 RazvanML commented on issue "Wallet passphrases silently ignore everything after a null character":
(https://github.com/bitcoin/bitcoin/issues/27067#issuecomment-1434642425)
Even if fixed, the change will bring a backwards compatibility problem. People who already have the passphrase containing a zero will experience a failure until they truncate the existing passphrase.

📝 furszy review_request_removed a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)
Structurally, the wallet class shouldn't access the global `ArgsManager` class, its internal behavior shouldn't be coupled to a global command line args parsing object.

So this PR migrates the only two places where we depend on it: (1) the keypool size, and (2) the "-walletnotify" script. And cleans up the, now unneeded, wallet `ArgsManager` ref member.

Extra note:
In the process of removing the args ref member, discovered and fixed files that were invalidly depending on the wallet header
...