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

💬 fanquake commented on pull request " rpc/wallet: Add details and duplicate section for simulaterawtransaction":
(https://github.com/bitcoin/bitcoin/pull/25621#issuecomment-1434761587)
@anibilthare are you still working on this? Can you comment where you've addressed the review feedback. Note that at least one test is also failing.

💬 fanquake commented on pull request "Optimizations & simplifications following #25717":
(https://github.com/bitcoin/bitcoin/pull/25968#issuecomment-1434766905)
@sipa are you inteterested in adressing the review feedback here?

💬 brunoerg commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1109955578)
Yea, nice find, thanks!

💬 theuni commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1434792804)
ACK 4faa4e37a6511c6ada303ef7929ac99c7462f083. After playing with this quite a bit I didn't observe any noticeable pitfalls.

I'd expect this to matter more in low-level code, so I'm interested to see how much #27118 affects the numbers.

💬 theuni commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434795409)
Do you have a before/after count for hardened functions?

💬 fanquake commented on pull request "contrib: add ELF OS ABI check to symbol-check.py":
(https://github.com/bitcoin/bitcoin/pull/26953#issuecomment-1434803116)
Need to follow up here in regards to powerpc64 vs le. le is currently 3.10.0:
```bash
ELF 64-bit LSB pie executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld64.so.2, for GNU/Linux 3.10.0, with debug_info, not stripped
```
as opposed to non-le:
```bash
ELF 64-bit MSB pie executable, 64-bit PowerPC or cisco 7500, Power ELF V1 ABI, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld64.so.1, for GNU/Linux
...

💬 fanquake commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1434820512)
> Do you have a before/after count for hardened functions?

Using GCC 13 and glibc 2.37 (with only branch):
```bash
# master
nm -C src/bitcoind | grep _chk
U __fprintf_chk@GLIBC_2.17
U __memcpy_chk@GLIBC_2.17
U __snprintf_chk@GLIBC_2.17
U __stack_chk_fail@GLIBC_2.17
U __stack_chk_guard@GLIBC_2.17
U __vsnprintf_chk@GLIBC_2.17
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
32

#
...