💬 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());
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144072741)
```suggestion
// can't use resource because alignment is too big, allocate system memory
b = resource.Allocate(8, 16);
BOOST_TEST(block == b);
block = b;
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144070292)
nit, and would this make the test too specific? I don't think so, but something to consider.
```suggestion
void* b = resource.Allocate(8, 1);
BOOST_TEST(block == b);
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144079597)
`counts` is unused

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144076687)
```suggestion
// can't use resource because size is too big, allocate system memory
b = resource.Allocate(16, 8);
BOOST_TEST(block != b);
block = b;

```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144084403)
```suggestion
// set each byte to num_bytes
```

💬 theStack commented on pull request "Remove confusing "Dust" label from coincontrol dialog":
(https://github.com/bitcoin-core/gui/pull/719#issuecomment-1478817516)
> While you are here, could also align the "bytes" and "change" labels to left. Feel free to squash this: [furszy/bitcoin-core@53165e3](https://github.com/furszy/bitcoin-core/commit/53165e3cc02cf4fdff2b274ad98e1868196e9644)

Could you post a screenshot on how dialogs look before/after this change? On my end this doesn't change anything and the alignment seems to be fine even on master.