💬 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

#
...

💬 MarcoFalke commented on pull request "lint: enable E722 do not use bare except":
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1434851767)
Yeah, I was thinking it would be easier to replace them with `except Exception`, like you did initially. This would mean the review is easy, because it can only affect `KeyError`, right? With several exceptions listed, we'd have to review the whole call stack, which is burdensome, no?

💬 willcl-ark commented on issue "TransactionError::ALREADY_IN_CHAIN inconsistency ":
(https://github.com/bitcoin/bitcoin/issues/19363#issuecomment-1434852432)
Although it is inconsistent I slightly prefer the idea of returning the most accurate information available to the user.

Following this logic instead of removing this error type, we could take two new actions to try and increase the chance that the caller gets more accurate information back:

1. Inside `BroadcastTransaction()` add a call to `GetTransaction()` on the condition that the user has `txindex` enabled
2. Augment [`HandleATMPError`](https://github.com/bitcoin/bitcoin/blob/fe1b3256
...