💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1143958464)
Done with one more tweak. I'll use an RAII wrapper if I have to touch the code again, and if that's the case, could you pls point me to an example already here in the bitcoin core code if already exists? Thanks.

💬 brunoerg commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1478543142)
Rebased

💬 brunoerg commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1478551323)
> Something like: connect to one generic block-relay-only peer, plus an additional block-relay-only peer for every network we support? I think that should only improve things, including during IBD. It wouldn't do anything to improve tx propagation of course, so wouldn't make this PR redundant.

Also, should we have (at least) one anchor per network instead of only two generic ones?

💬 TheCharlatan commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#issuecomment-1478567703)
ACK e930814fdb9261f180d5c436cefe52e9cf38fd67 fwiw :P
Thank you for following up on this @mzumsande.

💬 TheCharlatan commented on pull request "Refactor: Remove unused FlatFilePos::SetNull":
(https://github.com/bitcoin/bitcoin/pull/27289#issuecomment-1478572346)
ACK fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a

💬 achow101 commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1478584229)
ACK 95ad70ab652ddde7de65f633c36c1378b26a313a

💬 TheCharlatan commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1478594074)
re-ACK https://github.com/bitcoin/bitcoin/commit/95ad70ab652ddde7de65f633c36c1378b26a313a

📝 TheCharlatan opened a pull request: "refactor: Move chain names to the kernel namespace"
(https://github.com/bitcoin/bitcoin/pull/27294)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is also a follow up to #26177.

The code move of the chain name constants out of the `chainparamsbase` to their own separate header allows the kernel `chainparams` to no longer include `chainparamsbase`. The `chainparamsbase` contain references to
...

💬 achow101 commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1478663359)
Light ACK d87cb99bb37637e26a9e00b9f7de4bc6f44cb79d

Not particularly well versed in allocators, but the logic of the allocator and its tests makes sense, and the benchmarks seem to show there is a noticeable improvement.

One thing I did notice though is that when configured with `--enable-debug`, the benchmark `PoolAllocator_StdUnorderedMapWithPoolResource` is a little bit slower than `PoolAllocator_StdUnorderedMap`. Without `--enable-debug`, it's quite a bit faster. I didn't measure the ef
...

💬 ishaanam commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1478682109)
> However given that people seem to think the blank flag has more meaning behind it, I'm okay with dropping the change to descriptor wallets.

I think dropping the change to descriptor wallets would make more sense here. Personally I have always thought that `blank` for descriptor wallets meant that the wallet does not contain any wallet-generated descriptors. Additionally, as @S3RK pointed out, this would allow for maintaining the current behavior until a new flag can be added.

💬 ajtowns commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144052536)
This makes a copy of the name in every module that includes this header, which seems backwards, even if they end up getting merged by the linker...

💬 ajtowns commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144081960)
Might be easier to review if these were scripted-diffs? Something like https://github.com/ajtowns/bitcoin/commits/202303-pr27294split perhaps

📝 brunoerg opened a pull request: "test: add support for all networks in `deserialize_v2`"
(https://github.com/bitcoin/bitcoin/pull/27295)
Fixes #27140

💬 jamesob commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1478764099)
Back with results for dbcache=1000; less noticeable speedup (5%) and increased memory usage (11%).

---


| bench name | command
...

💬 davidgumberg commented on pull request "test: Fix TypeError (expected str instance, bytes found) in wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/27280#issuecomment-1478788195)
tACK https://github.com/bitcoin/bitcoin/pull/27280/commits/33337eb86028662e632107411efec1e63b1c01bf

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144023009)
```suggestion
assert(free_block.size <= chunk_size_remaining); // ensure no overflow
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144025036)
```suggestion
std::size_t alignment = std::size_t{1} << InsecureRandRange(8); // 1, 2, ..., 128
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144025651)
```suggestion
for (const std::byte* ptr : resource.m_allocated_chunks) {
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144046537)
Probably not important, but if a bug caused a cycle in a freelist, I think the test would allocate an unbounded amount of memory (pushing to `free_blocks`), which would be not a nice way to fail. I think you could calculate an upper bound of the number of free blocks (number of chunks times chunk size divided by this free list's blocksize), then assert if the number of iterations exceeds that number.

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144026131)
nit, this would test slightly more
```suggestion
assert(chunk_ptr_remaining == chunk_it->ptr + chunk_it->size); // ensure we are at the end of the chunks
++chunk_it;
assert(chunk_it == chunks.end());
```