💬 hebasto commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#discussion_r1110114839)
> ... can be omitted if \<condition\>

It creates additional burden for reviewing if \<condition\> will change in the future, doesn't it?

💬 achow101 commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#issuecomment-1435007548)
ACK 52f4d567d69425dfd514489079db80483024a80d

🚀 achow101 merged a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)

💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1435034303)
For anyone wanting to review this PR and would like some help with basic mempool concepts, I made a video: https://youtu.be/sQ05azzTp9o -- it mentions 26152 but I think would be helpful for reviewers here as well.

💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1110150658)
This is good feedback, and in the general case you're right. In the specific case of the deferred checks being introduced here, they don't include any of the computationally expensive stuff that motivates parallelizing checks in the case of CScriptCheck - i.e. we're not doing any cryptographic ops or signature checking here; OP_VAULT deferred checks consist of summing and comparing integers.

So I'm not necessarily averse to doing the deferred-check parallelization in this changeset, but (i) I
...

💬 LarryRuane commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1435036691)
For anyone wanting to review this PR and would like some help with basic mempool concepts, I made a video: https://youtu.be/sQ05azzTp9o.

💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1110154588)
I looked at using `std::variant` but the end use in `ValidateDeferredChecks` winds up being much hairier. I've changed the specific checks from `std::unique_ptr` to `std::optional` for simplicity's sake. If we eventually decide that the memory implications of this approach are too heavyweight, we could do something with std::variant - but I think they're comparable, and given that we'll never have more than a few thousand deferred checks in flight at any given time, I think it's probably okay to
...

👍 john-moffett approved a pull request: "refactor: remove windows-only compat.h usage in random"
(https://github.com/bitcoin/bitcoin/pull/26814)
ACK 621cfb77227b5a240d66547947f73130f0c51f44

💬 jonatack commented on pull request "contrib: remove install_db4.sh":
(https://github.com/bitcoin/bitcoin/pull/26834#issuecomment-1435058468)
Made a number of updates to https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests that include this change.

⚠️ GaloisField2718 opened an issue: "error: timeout on transient error: Could not connect to the server 127.0.0.1:8333 (error code 1 - "EOF reached")"
(https://github.com/bitcoin/bitcoin/issues/27121)
<!-- This issue tracker is only for technical issues related to Bitcoin Core.

General bitcoin questions and/or support requests are best directed to the Bitcoin StackExchange at https://bitcoin.stackexchange.com.

For reporting security issues, please read instructions at https://bitcoincore.org/en/contact/.

If the node is "stuck" during sync or giving "block checksum mismatch" errors, please ensure your hardware is stable by running memtest and observe CPU temperature with a load-test t
...

💬 achow101 commented on pull request "net: avoid overriding non-virtual ToString() in CService and use better naming":
(https://github.com/bitcoin/bitcoin/pull/25619#issuecomment-1435065439)
ACK c9d548c91fb12fba516dee896f1f97692cfa2104

💬 brunoerg commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1110179294)
Yes, better to call `LookupSubNet()`.

💬 brunoerg commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1110180280)
Done

💬 brunoerg commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1110180577)
Make sense, done!

💬 brunoerg commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#discussion_r1110180782)
Done!

💬 brunoerg commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#issuecomment-1435068141)
Force-pushed addressing @vasild's review.

💬 brunoerg commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1435079192)
> Subnetting does not make sense for tor/i2p/cjdns
> It follows that banman is doing something that does not make sense (e.g. subnetting tor). This is the root of the problem. I think if that is eradicated, then the rest will untangle by itself.

`CConnman::DisconnectNode` also handles addresses as subnets. Is this a problem?

🚀 achow101 merged a pull request: "net: avoid overriding non-virtual ToString() in CService and use better naming"
(https://github.com/bitcoin/bitcoin/pull/25619)

💬 yancyribbens commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1435080650)
@jonatack thanks for the response. I agree with you that feedback is not just a technical process but also a social one. You're links also help as well.

@achow101 @MarcoFalke thanks for taking the time to review.

💬 yancyribbens commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1435081051)
s/you're/your