💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101324949)
Concept ACK

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101333852)
I think it is worth mentioning that the current approach is defining the minimum amount of file descriptions required without accounting for a single connection*, this means that if we are at the bare minimum, we will run but won't be able to connect to any nodes. This is consistent with the current logic in master, but I think it's not the way it should be.

I'm happy to add another commit addressing this, but I've rathered start approaching this sticking to the same assumptions as master.

...

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2101340385)
@vasild you may be interested in this. I decided to fix it when seeing you extending the current logic in #29415

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1594569600)
Hmm, if what you need is a wrapped `keypair`, how about creating a wrapped `keypair`? :) https://github.com/theuni/bitcoin/commit/1a1599f59f711206ea91f025ee48b148e17b01ce

^^ Completely untested. Maybe state via `ApplyTapTweak` is unnecessary as the ctor could just take a pointer to a merkle tree instead?

💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#issuecomment-2101342201)
Force-pushed to address compilation failure on non-macOS systems:
- moved `GetNodeWarnings()` (in `rpc/util.cpp`) to `node::GetWarningsForRpc()` (in `node/warnings.cpp`). Since `rpc/util.cpp` is in `common`, this causes issues after warnings are moved to `node`. I don't love this approach, but it seemed like the least bad one - open to suggestions if anyone has any?
- updated bitcoin-chainstate.cpp to the new `kernel::Notifications` interface

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1594573095)
Yes, in the reindex case; In this case the passed dbp isn't changed (it's now a const arg to `AddToBlockFileInfo`). If a dpb was passed to AcceptBlock for which `dbp->IsNull()`, the error message ("Failed to find position to write new block to disk") would have been very confusing anyway, because we don't write a block to disk during reindex anyway.

💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594581007)
nit: `self.nodes[0]` -> `node`

👋 paplorinc's pull request is ready for review: "refactor: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing"
(https://github.com/bitcoin/bitcoin/pull/29473)

💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594592389)
> it checks for three concerns

That's a property based test, i.e. for random valid inputs it checks that certain conditions hold - in this case a roundtrip, that decoding an encoded results in the original trimmed value.
It's meant to find corner cases that we haven't though of, that's its single concern, we have separate tests for each corner case we know about.

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594592618)
Covered in [1ed818a](https://github.com/bitcoin/bitcoin/pull/29605/commits/1ed818a35d78a5bf6eaf0f72f3730041913b4859)

💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594596029)
The file was already using this format: https://github.com/bitcoin/bitcoin/blob/master/src/test/base58_tests.cpp#L74

💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594600115)
Valid concern, I also testes it to understand why it was done differently

💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594602235)
Thanks for your detailed review, will do that this week

💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594614105)
Still failing on 1ed818a35d78a5bf6eaf0f72f3730041913b4859 for me.
[29605_timeout_1ed818a.combined_log.gz](https://github.com/bitcoin/bitcoin/files/15254533/29605_timeout_1ed818a.combined_log.gz)

### Excerpts

Startup:
```
test 2024-05-08T20:37:27.879000Z TestFramework (INFO): PRNG seed is: 2801220528353188963
test 2024-05-08T20:37:27.879000Z TestFramework (DEBUG): Setting up network thread
```
~8 seconds later we make the attempt to connect to the bogus IP:
```
node0 2024-05-08T
...

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594643915)
That's interesting. I guess it's still too close.

```
node0 2024-05-08T20:37:35.200946Z [opencon] [net.cpp:2501] [ThreadOpenConnections] Fixed seeds are disabled
node0 2024-05-08T20:37:35.254287Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:60322
node0 2024-05-08T20:37:35.254724Z [httpworker.0] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=getblockcount user=__cookie__
node0 2024-05-08T20:37:35.256529Z [http] [httpserve
...

📝 instagibbs opened a pull request: "test: add conflicting topology test case"
(https://github.com/bitcoin/bitcoin/pull/30066)
We want to ensure that even if topologies
that are acceptable are relaxed, like
removing package-not-child-with-unconfirmed-parents, that we don't end up accepting packages we shouldn't.

💬 instagibbs commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#issuecomment-2101470172)
cc @glozow

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594702190)
Had to check whether `UniValue` even had a move-constructor. It seems like one should be generated implicitly if my readings of https://en.cppreference.com/w/cpp/language/move_constructor#Implicitly-declared_move_constructor and `univalue.h` are correct.

@ryanofsky's explanation rings true with my long underutilized C++11 neurons.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594704577)
Looks more like a typo, see lines 67 and 78.

🚀 achow101 merged a pull request: "rpc: parse legacy pubkeys consistently with specific error messages"
(https://github.com/bitcoin/bitcoin/pull/28336)