💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3532157521)
From https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3528997008 I gather that we should not reuse the method ordinals. Since cap&proto doesn't allow gaps, when deprecating methods, it's best to just have them throw an error.

In https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3529071835 @ryanofsky suggested replacing the dummy coinbase with an empty transaction, so that clients are not tempted to use the dummy coinbase via `getBlock()` or `getCoinbaseTx()`.

I think
...

🤔 frankomosh reviewed a pull request: "net: Filter addrman during address selection via AddrPolicy to avoid underfill"
(https://github.com/bitcoin/bitcoin/pull/33663#pullrequestreview-3464332521)
Concept ACK

💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527033639)
Is there a way/test to verify that the `BanMan` methods here are safe to call while `AddrMan`'s lock is held?

Can see a comment in `addrman.h` says the policy "must not acquire locks that can conflict with `AddrMan::cs`". So it would make sense to ensure deadlock scenario is not accidentally created.

💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527062353)
Would network mismatches also count as `filtered` i this case?

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2527112930)
Ah yes, that is cleaner.

💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527173880)
> that's literally English for data_dir == nullptr && data_dir_len > 0 evaluating to true, therefore satisfying the if condition, therefore triggering the early return.

The `&&` requires both of conditions to be true to process the early return branch? The way you had it written before is confusing because if the ptr is null, it is a logical conclusion that data_dir_len should also be zero. So, either `data_dir == nullptr` alone is enough, or `data_dir == nullptr || data_dir_len == 0` to be
...

👍 stickies-v approved a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3464527742)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362

💬 TheCharlatan commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527219768)
I don't think it does, afaik a write exception through `Sync` or `Flush` is just bubbled up and then logged. But I might be missing something.

💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3532419886)
> I think I'm going to combine these two observations and make this PR a breaking change. Marking as draft for now.

This seems ok, but isn't really what I would suggest. My suggestion would be for this PR to just introduce a new `getCoinBase()` method like it's doing and not remove other methods so existing mining clients do not immediately stop working. The other methods, which are all simple accessors, can easily be removed later.

Adding a new method now and removing older methods later
...

💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527283208)
> The way you had it written before is confusing because if the ptr is null, it is a logical conclusion that data_dir_len should also be zero.

*should* being the keyword here indeed. A non-zero length with a nullptr indicates the user clearly made an error, regardless of business logic. We need to return early in order to not cause any UB later on, and because the request is nonsensical. In the earlier approach on which you commented (0ae6a22a9f44b7e609431b9bac8bcf1b41d9a977), nullptr (but _o
...

✅ hebasto closed an issue: "Implicit conversion from `fs::path` to `std::string` when constructing file streams"
(https://github.com/bitcoin/bitcoin/issues/33545)

💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3532476657)
Fixed in https://github.com/bitcoin/bitcoin/pull/33550.

💬 willcl-ark commented on pull request "depends: sqlite 3.51.0; switch to autosetup":
(https://github.com/bitcoin/bitcoin/pull/32655#issuecomment-3532580690)
Build:

```
❯ uname -m; find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
x86_64
3e2443f9828f1541c02f5c33d29c78945bf218411bc35cf13894ad048495aad2 guix-build-1db74914706f/output/aarch64-linux-gnu/SHA256SUMS.part
4a0f92a547fb3f94f930cbdb0575aaab16b1f7d95cd9172cce44068e1ec7ff62 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu-debug.tar.gz
c98f76eae0da3bf25774f5e8b25edac8d315bc428b2e21
...

🤔 hebasto reviewed a pull request: "depends: drop Qt patches"
(https://github.com/bitcoin/bitcoin/pull/33860#pullrequestreview-3464798591)
Post-merge ACK.

I tested using GCC 12 with CMake 3.22 and 3.27.

💬 stringintech commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3532613361)
By inconsistency, I mean that refactoring `set_options`/`set_level_category`/`enable_category`/etc. to receive a `btck_LoggingConnection*` parameter suggests that settings are supported per connection, while calling them on a newly instantiated logging connection would actually override settings for all previously instantiated connections. While the actual behavior is well-documented, the function signatures themselves may still create expectations of per-connection isolation.

I understand yo
...

💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527423684)
The BanMan methods used here take `m_banned_mutex` and never call back into `AddrMan`, so the lock order in this policy is currently `AddrMan::cs`>`BanMan::m_banned_mutex` with no reverse lock order in the codebase.

Using the predicate here also enforces that `AddrMan::cs` is always taken first, before any `BanMan` locks which helps maintain a consistent lock order.

`GetAddressesUnsafe()` is invoked on startup to load `peers.dat` with the responses being cached for `CConnman::GetAddresses(
...

💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527436474)
Yes; Currently all skipped addresses in `AddrMan::GetAddr` are counted as `filtered`; If you'd prefer (or find it useful) separate counters for each reason (Network, Quality or Policy), I can add that.

🤔 rkrux reviewed a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3464805989)
Concept ACK 0cbcdcaf5a7fe1dd864d0019a7d7e4f8b80b5e7c

Makes sense to use the pre-mined chain and the MiniWallet for this use case.

💬 rkrux commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2527400928)
In d1a2e278572f70d0d68a21312b2cc7de27f326df "test: replace class ValidWitnessMalleatedTx with function"

Unlike the `malleate_tx_to_invalid_witness` function that is quite generic (https://github.com/bitcoin/bitcoin/pull/33793/files#diff-1698fabd4ddd298eb28e234d97f34e744f568852b77361384c8edb38819de5e3R259), this function at the moment doesn't seem generic enough to be present in an util file. Few points regarding the function I noticed:

1. The name doesn't reflect the implementation, the fu
...

💬 rkrux commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2527403577)
In 26577cac8b9cf53d5f69e4502346fd5881c92004 "test: use pre-generated chain"

Nit (not strong opinion): Can remove this line altogether as well because `False` is the default value.