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

💬 jarolrod commented on pull request "Adjust plural forms for translations":
(https://github.com/bitcoin-core/gui/pull/716#issuecomment-1478968364)
concept ack

💬 TheCharlatan commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1144287702)
Yeah totally, not sure what I was thinking here.
Please resolve this.

💬 nopara73 commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1479058400)
For Wasabi's purposes this output is needed purely for fee estimation, so it'd be ideal if Core would estimate fees properly. Here's how I see things:

- `estimatesmartfee` estimates fees based on the **PAST**
- This PR enables us to put minimums on Core's fee estimations. Eg. if Core estimates 6 hours at 10 sat/b, based on the past, but the mempool is already filled with 12 hours of 11 sat/b estimations, then what we do (currently with Knots) is that based on the mempool histogram we upgrade
...

💬 TheCharlatan commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144421842)
I can make `chainname` a struct instead and use the same pattern as it was before in `chainparamsbase`, but I'm not sure if this is so much better. Isn't the default string constructor called anyway for each time this header is included? How about taking this a step further and making them a `const std::string_view` instead? I pushed https://github.com/TheCharlatan/bitcoin/commit/1e4ba77db8121f4bc58653c786595325b308ac31 as a proof of concept. Looking at the diff, I'm not sure if it is worth it t
...

💬 TheCharlatan commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144421895)
Thank you for this concrete split suggestion. I did not add a scripted diff, to show the change of header and definition namespacing in the same commit. This makes it easier to see that for every changed file the new header is included as well. To me this feels like it will make for a cleaner commit history. However, I'm new to contributing and reviewing large code moves to the project. I'll ponder over your suggested split, I usually come around to these suggestions after a bit :)