💬 mzumsande commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1143907496)
I switched to your suggestion, removing the assert!

💬 instagibbs commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1143918290)
hmmm, I don't recall. I can just set to 0

💬 instagibbs commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1143923923)
will remove

💬 instagibbs commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1143932896)
looks like something that doesn't matter, based on me getting rid of that factor. Removed.

💬 instagibbs commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#issuecomment-1478511512)
fixups included, squashed

💬 theuni commented on pull request "build: Link `libbitcoinkernel` to `libbitcoin_util`":
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1478520029)
Agree with the NACKs here. It would be nice if we could share the objs to reduce compilation time, but there are good reasons to keep things separate.

But to add another practical problem, eventually libbitcoinkernel will need to be built with a `-DLIBBITCOINKERNEL_SHARED` or so once it gains an API. That would mean that objects are no-longer identical to (or shareable with) our internal libs.

💬 dhruv commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1143941594)
@fanquake not sure I understand. At which commit is the build broken? I specifically did it in a way that avoided that but may be misunderstanding something.

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1143951406)
@dhruv ah, my misread was that you'd left the linker errors between commits 2 & 4. Although, I think introducing this, just to change in later commits is still not ideal. Will re-mark this as resolved, and post new review below.

💬 achow101 commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1478541182)
ACK 730bff20c1f76c46a7ab6c8d981126fe3ad2cec9

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