💬 MarcoFalke commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1275046063)
Is this faster than the setup in `./src/wallet/test/fuzz/notifications.cpp`? If yes, could switch that one over as well?

👍 john-moffett approved a pull request: "refactor: Remove C-style const-violating cast, Use reinterpret_cast"
(https://github.com/bitcoin/bitcoin/pull/28127#pullrequestreview-1547929186)
ACK fa9108f85afdc926fd6a8b96cc2acff7ca25d7a8

👋 MarcoFalke's pull request is ready for review: "ci: Move ASan USDT to persistent_worker"
(https://github.com/bitcoin/bitcoin/pull/28161)

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1651962091)
@jamesob Thanks for your comments, I think I largely agree.

It's a good observation that the case where a large non-compact block message is necessary for block connectivity is already fairly pessimal. Some more points to put this overhead in perspective:
* If you assume - say - a 1 Gbit/s connection, then receiving a byte takes 8 ns already, so adding 1.6 ns at most adds ~20% (regardless of whether the received data is short or big), and this ignores any other processing on top for the new
...

💬 MarcoFalke commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1651975910)
cc @0xB10C about the second commit

💬 stickies-v commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1275110654)
I'm not really keen on setting limits. I don't know what good values would be, and ultimately it's not a huge problem if the value is too high. I think issuing a warning could be sensible as it does have meaningful impact, but I don't have a strong view. Too many warnings/popups is also not great UX.

For example, this would log a warning in the concole, and on qt issue a warning pop-up if a value > 100x default value is entered:

```diff
diff --git a/src/node/peerman_args.cpp b/src/node/pe
...

🚀 fanquake merged a pull request: "refactor: Remove C-style const-violating cast, Use reinterpret_cast"
(https://github.com/bitcoin/bitcoin/pull/28127)

💬 pinheadmz commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#issuecomment-1651999459)
Due to the multiple datadirs in the `feature_config_args` test, this assertion actually *does* fail in `combine_logs.py`:

https://github.com/bitcoin/bitcoin/blob/db7120fbaddff6037734d8e42d1d002d773f8567/test/functional/combine_logs.py#L86

I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1652004878)
> This is I think the biggest concern with a streaming API: the caller immediately gets (parts of) the decrypted message back, but is not allowed to use/inspect them in any way before the full message has been processed.

So just to be sure I understand, the risk is not any additional space requirement (since we need to buffer the full decrypted message anyway as things stand right now), but rather the risk of streaming is that the caller of this API will make some unwise use of a partially pr
...

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1652013883)
> So just to be sure I understand, the risk is not any additional space requirement (since we need to buffer the full decrypted message anyway as things stand right now), but rather the risk of streaming is that the caller of this API will make some unwise use of a partially processed message before it has been proven to be authentic.

Yes.

> If correct, this strikes me as a pretty remote risk given that the consumption of this API is limited to our codebase and so it should be easy to make
...

🤔 pinheadmz reviewed a pull request: "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain"
(https://github.com/bitcoin/bitcoin/pull/28157#pullrequestreview-1548023545)
concept ACK db7120fbaddff6037734d8e42d1d002d773f8567

Good follow up, addressed the TODOs leftover from #27622 well. Left a few comments

💬 pinheadmz commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1275138846)
You don't need to clear here, or on L125 because you are opening the conf file with "w" so the contents of the file get overwritten. Tests pass without the two clear lines, and I checked with extra open/read/print statements that the expected configuration settings were present in the file at each step.

💬 0xB10C commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1652061642)
ACK, looks like the tests [are run](https://cirrus-ci.com/task/6074291154845696?logs=ci#L3392).
```
interface_usdt_coinselection.py | ✓ Passed | 17 s
interface_usdt_mempool.py | ✓ Passed | 37 s
interface_usdt_net.py | ✓ Passed | 14 s
interface_usdt_utxocache.py | ✓ Passed | 35 s
interface_usdt_validation.py | ✓ Passed | 8 s
```

💬 vasild commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1652088394)
Reported at https://i2pgit.org/i2p-hackers/i2p.i2p/-/issues/399

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1652089161)
I guess allowing more connections beyond the max is a DoS vector, which could potentially crash a node and break all existing connections anyway... hm/

💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1652101826)
> [geti2p.net/en/download](https://geti2p.net/en/download) has just updated their available macOS version from 1.9 to 2.3 -- will retest.

FWIW, I didn't manage to make the v2.3 jar work and had to reinstall v1.9 (available as a dmg) at https://geti2p.net/en/download/mac. Per that page: *"Important Note: The 2.1.0 Mac OSX Easy Install Bundle release is delayed. Please install the 1.9.0 release below. You will be notified in the router console when the 2.1.0 update is available. Thank you for y
...

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1275188943)
> I think this should either be reverted, or the usdt requirement for root be dropped.

How would you feel about adding an environment variable to "force" root in `00_setup_env_native_asan.sh`? The functional test I added no longer skips if root (just expects a different behavior), so this actually would be kinda nice to have one task *does* run all functional tests as root.

💬 fanquake commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1652117028)
> FWIW, I didn't manage to make the v2.3 jar work and had to reinstall v1.9 (available as a dmg) at

I don't think that's something we'd want to advise anyone todo, given https://geti2p.net/en/blog/post/2023/01/31/mac-easy-install-notarization:
> 1.9.0 has known security issues and is not suitable for hosting services or any long-term use. Users are advised to migrate away as soon as possible. Advanced users of the Easy-Install bundle may work around this by compiling the bundle from source
...

💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1652135083)
Yes. I find the lighter-weight i2pd router in C++ to be an easier option for node operators to install: `apt install i2pd` / `brew install i2pd`, etc. That router did experience some unusual issues with flood attacks in June, but otherwise has been reliable for me over the past couple of years.

💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1275213995)
I think the contexts are different. `notifications` is working with descriptor and here is legacy, I believe I'll have something better when I finish the `DescriptorScriptPubKey` target. Also, I believe the `FuzzedWallet` in `notifications` is disabled for spkm.