👍 stickies-v approved a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3464527742)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
(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.
(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
...
(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
...
(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)
(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.
(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
...
(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.
(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
...
(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(
...
(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.
(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.
(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
...
(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.
(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.
💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527465258)
> A non-zero length with a nullptr indicates the user clearly made an error, regardless of business logic
This would occur not from a mistake made by the API consumer, but an error with the binding logic (in rust it would be an error with cc/bindgen) I think. That is to say, a rather improbable condition imo.
> In the earlier approach on which you commented (https://github.com/bitcoin/bitcoin/commit/0ae6a22a9f44b7e609431b9bac8bcf1b41d9a977), nullptr (but only with zero length) was allowed
...
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527465258)
> A non-zero length with a nullptr indicates the user clearly made an error, regardless of business logic
This would occur not from a mistake made by the API consumer, but an error with the binding logic (in rust it would be an error with cc/bindgen) I think. That is to say, a rather improbable condition imo.
> In the earlier approach on which you commented (https://github.com/bitcoin/bitcoin/commit/0ae6a22a9f44b7e609431b9bac8bcf1b41d9a977), nullptr (but only with zero length) was allowed
...
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527492985)
but aren't read and write symmetric now in that regard?
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527492985)
but aren't read and write symmetric now in that regard?
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3532765861)
Minor nits applied
<details>
<summary>git diff ea998e18f6d64f485d4e2d85a1028474fb35120a 02df44465218a49cc488fd50bf05ab2fd203c69c
</summary>
```diff
@@ -114,7 +114,7 @@ BOOST_AUTO_TEST_CASE(skip_height_properties_test)
for(int i{1}; i < n; ++i) {
auto new_index = std::make_unique<CBlockIndex>();
new_index->pprev = block_index.back().get();
- BOOST_CHECK(new_index->pprev);
+ BOOST_REQUIRE(new_index->pprev);
new_index->nHeight = new_index
...
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3532765861)
Minor nits applied
<details>
<summary>git diff ea998e18f6d64f485d4e2d85a1028474fb35120a 02df44465218a49cc488fd50bf05ab2fd203c69c
</summary>
```diff
@@ -114,7 +114,7 @@ BOOST_AUTO_TEST_CASE(skip_height_properties_test)
for(int i{1}; i < n; ++i) {
auto new_index = std::make_unique<CBlockIndex>();
new_index->pprev = block_index.back().get();
- BOOST_CHECK(new_index->pprev);
+ BOOST_REQUIRE(new_index->pprev);
new_index->nHeight = new_index
...
💬 TheCharlatan commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527544369)
Mmh, not sure what you are trying to say with that. What is the symmetry there? I think I am missing something, can you try and elaborate a bit?
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527544369)
Mmh, not sure what you are trying to say with that. What is the symmetry there? I think I am missing something, can you try and elaborate a bit?
💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527556716)
> This would occur not from a mistake made by the API consumer, but an error with the binding logic
Not all users will use language bindings, and regardless, I don't think the distinction between "direct usage" and "wrapped usage" is meaningful here. Our implementation has UB when a non-zero length is used with a nullptr, so our implementation is responsible for preventing the UB.
> a check for both null and empty could be done without checking length.
How? In the API, a string is defin
...
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527556716)
> This would occur not from a mistake made by the API consumer, but an error with the binding logic
Not all users will use language bindings, and regardless, I don't think the distinction between "direct usage" and "wrapped usage" is meaningful here. Our implementation has UB when a non-zero length is used with a nullptr, so our implementation is responsible for preventing the UB.
> a check for both null and empty could be done without checking length.
How? In the API, a string is defin
...
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527561867)
the comment states: "shutdown on LevelDB read errors [...] Writes do not need similar protection".
So now that reads and writes both throw and don't return, do we need to adjust the error catchers - since either reads also don't need protection anymore or writes also do - if I understand the catchers' purpose...
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527561867)
the comment states: "shutdown on LevelDB read errors [...] Writes do not need similar protection".
So now that reads and writes both throw and don't return, do we need to adjust the error catchers - since either reads also don't need protection anymore or writes also do - if I understand the catchers' purpose...