💬 fanquake commented on pull request "doc: docment json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#issuecomment-1459981209)
Concept ACK

💬 fanquake commented on pull request "test: Use self.wait_until over wait_until_helper":
(https://github.com/bitcoin/bitcoin/pull/27226#issuecomment-1459982082)
Concept ACK. Will test under the CI

💬 willcl-ark commented on issue "Option to ignore small inputs when internal wallet is building TXes?":
(https://github.com/bitcoin/bitcoin/issues/20870#issuecomment-1459986236)
@Crypto2 can you not just do the inverse and use coin selection/manual construction when you want to exclude small outputs?

💬 MarcoFalke commented on pull request "test: Default timeout factor to 4 under --valgrind":
(https://github.com/bitcoin/bitcoin/pull/27221#issuecomment-1459987786)
Not sure if that makes sense. This would require special casing the non-test binaries `bitcoind/-cli/-tx/-util` etc in the valgrind wrapper. Also, if someone wants to interactively re-run a single test inside the CI pod, they'd have to pass `--valgrind`. Maybe open a new discussion thread or pull request?

💬 fanquake commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1459992955)
> This triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529 ?

It will have been #27118 that triggered it. I can't look at this in mroe detail right now, but will by next week.

> On my Gentoo system with GCC 11.3.1 and glibc 2.36 this now gives me these warnings for each .cpp file when building Bitcoin Core

Looks like that's because you're using GCC 11.x, which doesn't support >=3 (it is supported with Clang >= 9). Somewhat annoying that glibc is going to warn like that,
...

💬 fanquake commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1129269455)
In 102b2033493f0d61e9763d094cb8a0017f7e3a10 bench: order the logging benchmark code by output:

Given that the code-movement, and re-ordering the `BENCHMARK(` calls doesn't change the output, this seems like a no-op?

In general, is (re-)ordering code in a `.cpp` file based on the order functions are called something we do?

💬 kristapsk commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1460007675)
> Looks like that's because you're using GCC 11.x, which doesn't support >=3 (it is supported with Clang >= 9). Somewhat annoying that glibc is going to warn like that, but they are also harmless, and fortification (level 2) is still being applied.

Could we somehow detect this at configure stage and apply fortification 3 only if it is supported?

💬 fanquake commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1460011191)
> Saw those for (maybe, maybe not) another pull; prefer to keep this small and focused.

If they are the same issue, what's the reasoning for leaving them for another PR? Not sure how the PR becomes less focused by fixing more of the same problem.

💬 fanquake commented on pull request "refactor: replace all implicit C-style const/const+reinterpret with explicit casts":
(https://github.com/bitcoin/bitcoin/pull/27126#discussion_r1129284068)
> That might make sense. If I get more time sometime soon, I'll see if I can easily span-ify it.

There doesn't seem to be much buy-in to making this change, and there's no real rush in any case, so feel free to look at using the Spans. Could probably mark this as a draft for now, if that is what you're going to look at.

💬 real-or-random commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1129293131)
```suggestion
static_assert(Num3072::BYTE_SIZE % 64 == 0);
ChaCha20Aligned(hashed_in.data(), hashed_in.size()).Keystream64(tmp, Num3072::BYTE_SIZE / 64);

```

💬 real-or-random commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1129311426)
(not this PR)

Should `SetIV()` also set `m_bufleft = 0`? If the caller sets an IV without seeking, and we have bytes left in the buffer, then the caller will get those remaining bytes which belong to the wrong buffer.

I mean, setting an IV without seeking is probably not a good idea anyway, and none of the callers currently do this. But I feel that's something that then should be excluded by the interface of the class, i.e., `SetIV()` and `SetKey()` should ideally seek to 0 (and perhaps be
...

💬 real-or-random commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1129318294)
Since this is a rekeying operation, consider `memory_cleanse`ing the cache buffer. (It will probably be overwritten soon anyway. but `SetKey()` should not happen frequently, so we don't need to worry about performance.)

I guess there should also be destructor that does the same. And (not this PR), a destructor of ChaCha20Aligned should `memory_cleanse` the key.

💬 real-or-random commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1129322207)
(not this PR)

nit: I'd prefer an interface that distinguishes between encryption and decryption. That makes the code easier to change if it will be reused for other ciphers.

💬 real-or-random commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1129313162)
(not this PR)

But I think initializing with an effective zero key should not be allowed by the interface of the class.

💬 fanquake commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#issuecomment-1460055753)
@real-or-random thanks for circling around to this post-merge.

💬 fanquake commented on issue "libevent: event_enable_debug_logging() not to be used after creating event base":
(https://github.com/bitcoin/bitcoin/issues/27182#issuecomment-1460060866)
Looks like, [according to the upstream reply](https://github.com/libevent/libevent/issues/1426#issuecomment-1455189774), this is not actually a problem at all, and the documentation is incorrect/outdated:

> I don't see any possible issues with calling `event_enable_debug_logging` after creating of `event_base`, and basically anytime (except for maybe a TSan warning, since the code is not uses proper atomics, but this should not be a problem, but it can leads to loosing few messages before/aft
...

💬 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_r1129354074)
Done

💬 Crypto2 commented on issue "Option to ignore small inputs when internal wallet is building TXes?":
(https://github.com/bitcoin/bitcoin/issues/20870#issuecomment-1460166098)
We already sweep them daily with scripts so they don't build up, would just be nice for them to not be spent automatically in between sweeps so it wouldn't have to be done as often.

💬 sipa commented on issue "Option to ignore small inputs when internal wallet is building TXes?":
(https://github.com/bitcoin/bitcoin/issues/20870#issuecomment-1460168607)
The coin selection algorithm should automatically avoid small inputs if they're uneconomical to spend (depending on feerate).

💬 hebasto commented on issue "libevent: event_enable_debug_logging() not to be used after creating event base":
(https://github.com/bitcoin/bitcoin/issues/27182#issuecomment-1460174988)
> So I think we can close this, and potentially follow up with documentation updates upstream?

Agree.