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

💬 RazvanML commented on issue "Wallet passphrases silently ignore everything after a null character":
(https://github.com/bitcoin/bitcoin/issues/27067#issuecomment-1434650979)
How about starting with a warning that the string contains a zero and only NN characters will be used? IMHO the backward compatibility issue is the major concern.

💬 vasild commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#issuecomment-1434668609)
Do you think that this is a blocker for this PR?

💬 ajtowns commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#issuecomment-1434721167)
> Do you think that this is a blocker for this PR?

I was just intending to note the difference not express an opinion (which I don't really have at this point). Would be nicer if the approach this PR recommends was clearly better in every way, though.

💬 fanquake commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434731916)
Changed the approach here, from using libevents own hardening option (might send some fixes upstream), and a patch, to just using `FORTIFY_SOURCE` directly. That gets as what we want at the moment, without other potential side effects.

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-1434735372)
> bitcoin-cli is fixed once we fortify libevent.

Rebased on #27118

💬 fanquake commented on pull request "refactor: don't avoid sys/types.h when building for Windows":
(https://github.com/bitcoin/bitcoin/pull/27098#issuecomment-1434738735)
Just going to combine this into #26832.

✅ fanquake closed a pull request: "refactor: don't avoid sys/types.h when building for Windows"
(https://github.com/bitcoin/bitcoin/pull/27098)

👋 fanquake's pull request is ready for review: "compat: move (win) S_* defines into bdb"
(https://github.com/bitcoin/bitcoin/pull/26832)

💬 brunoerg commented on issue "p2p_disconnect_ban intermittent issue":
(https://github.com/bitcoin/bitcoin/issues/26808#issuecomment-1434751546)
```py
self.log.info("disconnectnode: successfully disconnect node by address")
address1 = self.nodes[0].getpeerinfo()[0]['addr']
self.nodes[0].disconnectnode(address=address1)
self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 1, timeout=10)
assert not [node for node in self.nodes[0].getpeerinfo() if node['addr'] == address1]
```

We call `disconnectnode` and check if it worked by `getpeerinfo`. Perhaps, internally is it updating the informations before really compliting the dis
...

💬 fanquake commented on pull request "test: add coverage for rpc error when trying to rescan beyond pruned data":
(https://github.com/bitcoin/bitcoin/pull/25937#issuecomment-1434757278)
> @aureleoules, I think so.

Does that mean you are working on / implementing this?

💬 fanquake commented on pull request "test: fix test abort for high timeout values (and `--timeout-factor 0`)":
(https://github.com/bitcoin/bitcoin/pull/25950#issuecomment-1434759730)
Not clear what the status of this is. Looks like there are atleast some comments that need addressing?