💬 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.

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1144188630)
why not? `DataStream` is defined there.

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1144191381)
k, either way is fine for me. Cool about the special case exception removal and the new test.
just updated 27195 with further tests too.

👍 vasild approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
ACK 60c3f4918190900e5f79341abcc0878214656257

💬 vasild commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1144224404)
This is repeated in a few places and can be deduplicated (untested):

```cpp
void PushWarnings(const std::vector<bilingual_str>& warnings, UniValue& to)
{
if (IsDeprecatedRPCEnabled("warningfield")) {
// The "warning" field is deprecated in v25 for removal in v26.
to.pushKV("warning", Join(warnings, Untranslated("\n")).original);
}
if (warnings.empty()) {
return;
}
to.pushKV("warnings", UniValue{UniValue::VARR});
for (const auto& s : w
...

💬 vasild commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1478914811)
> we have authenticated connections ... CJDNS(?)

Yes, in CJDNS the address is (a representation) of the owner's public key.

> there's no relationship between the address and our route?

My understanding is that this is true for Tor, I2P and CJDNS.