💬 stickies-v commented on pull request "refactor: validation: mark CheckBlockIndex as const":
(https://github.com/bitcoin/bitcoin/pull/32364#discussion_r2075528317)
Oh yes, keeping only `self` is a better approach, thanks! Marking as resolved because the commit has been [dropped](https://github.com/bitcoin/bitcoin/pull/32364?notification_referrer_id=NT_kwDOBB0EGbQxNjExMTQxNTE5MDo2OTAxMDQ1Nw#issuecomment-2854656467).
(https://github.com/bitcoin/bitcoin/pull/32364#discussion_r2075528317)
Oh yes, keeping only `self` is a better approach, thanks! Marking as resolved because the commit has been [dropped](https://github.com/bitcoin/bitcoin/pull/32364?notification_referrer_id=NT_kwDOBB0EGbQxNjExMTQxNTE5MDo2OTAxMDQ1Nw#issuecomment-2854656467).
👍 willcl-ark approved a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2818380798)
ACK 22cff32319de64cb98e1c89b9a7ed35611e89e27
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2818380798)
ACK 22cff32319de64cb98e1c89b9a7ed35611e89e27
💬 andrewtoth commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2075559595)
Sorry, what is the original cache-usage-update? AFAICT this line was added in this PR, and there is no cache-usage-update here?
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2075559595)
Sorry, what is the original cache-usage-update? AFAICT this line was added in this PR, and there is no cache-usage-update here?
💬 laanwj commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075574236)
First thought this change was a bit strange, but "lose all your bitcoin" singular might make more sense even in English?
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075574236)
First thought this change was a bit strange, but "lose all your bitcoin" singular might make more sense even in English?
💬 McBeThC commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2854739598)
Concept NACK
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2854739598)
Concept NACK
💬 laanwj commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075580265)
This update looks correct, but we should really avoid having format codes in the translations, `%n` substitutes combined with HTML make it doubly confusing for translators.
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075580265)
This update looks correct, but we should really avoid having format codes in the translations, `%n` substitutes combined with HTML make it doubly confusing for translators.
💬 laanwj commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075591796)
Another translation with HTML in it.
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075591796)
Another translation with HTML in it.
💬 hebasto commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075606081)
This comes from:https://github.com/bitcoin/bitcoin/blob/f490f5562d4b20857ef8d042c050763795fd43da/src/qt/walletcontroller.cpp#L94
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075606081)
This comes from:https://github.com/bitcoin/bitcoin/blob/f490f5562d4b20857ef8d042c050763795fd43da/src/qt/walletcontroller.cpp#L94
📝 TheCharlatan opened a pull request: "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store"
(https://github.com/bitcoin/bitcoin/pull/32427)
Opening this PR mostly to get concept/approach feedback.
This is motivated by the kernel library, where the internal usage of leveldb is a limiting factor to its future use cases. Specifically it is not possible to share leveldb databases between two processes. A notable use-case for the kernel library is accessing and analyzing existing block data. Currently this can only be done by first shutting down the node writing this data. Moving away from leveldb opens the door towards doing this in
...
(https://github.com/bitcoin/bitcoin/pull/32427)
Opening this PR mostly to get concept/approach feedback.
This is motivated by the kernel library, where the internal usage of leveldb is a limiting factor to its future use cases. Specifically it is not possible to share leveldb databases between two processes. A notable use-case for the kernel library is accessing and analyzing existing block data. Currently this can only be done by first shutting down the node writing this data. Moving away from leveldb opens the door towards doing this in
...
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2075611741)
I prefer code over comments - and since the usual pattern is basically:
```C++
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
if (cursor.WillErase(*it)) {
itUs->second.coin = std::move(it->second.coin);
} else {
itUs->second.coin = it->second.coin;
}
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
```
it makes sense to explain (via code) why here we don't need the first part:
```C++
assert(entry.coin.DynamicMemoryUsage() == 0); // i.e. we didn't for
...
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2075611741)
I prefer code over comments - and since the usual pattern is basically:
```C++
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
if (cursor.WillErase(*it)) {
itUs->second.coin = std::move(it->second.coin);
} else {
itUs->second.coin = it->second.coin;
}
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
```
it makes sense to explain (via code) why here we don't need the first part:
```C++
assert(entry.coin.DynamicMemoryUsage() == 0); // i.e. we didn't for
...
💬 hebasto commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075613658)
This comes from: https://github.com/bitcoin/bitcoin/blob/f490f5562d4b20857ef8d042c050763795fd43da/src/qt/walletcontroller.cpp#L251-L253
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075613658)
This comes from: https://github.com/bitcoin/bitcoin/blob/f490f5562d4b20857ef8d042c050763795fd43da/src/qt/walletcontroller.cpp#L251-L253
💬 laanwj commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075614393)
Looks correct "horas" means hours, and this isn't about hours. It is introducing a redundant space before the dot here.
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075614393)
Looks correct "horas" means hours, and this isn't about hours. It is introducing a redundant space before the dot here.
💬 laanwj commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075622675)
Right! At some point we should refactor entries like this to
```c++
box.setText(tr("Are you sure you wish to close the wallet %1?").arg("<i>" + GUIUtil::HtmlEscape(wallet_model->getDisplayName() + "</i>")));
```
But not for this version ofc.
(https://github.com/bitcoin/bitcoin/pull/32352#discussion_r2075622675)
Right! At some point we should refactor entries like this to
```c++
box.setText(tr("Are you sure you wish to close the wallet %1?").arg("<i>" + GUIUtil::HtmlEscape(wallet_model->getDisplayName() + "</i>")));
```
But not for this version ofc.
👍 laanwj approved a pull request: "[29.x] qt: 29.1 translations update"
(https://github.com/bitcoin/bitcoin/pull/32352#pullrequestreview-2818619507)
ACK fc60337733a9dffaa42e08fcbff0ab24b5f679a4
i looked over the changes and did some spot checks and found nothing that looks malicious or appears out of the ordinary.
(https://github.com/bitcoin/bitcoin/pull/32352#pullrequestreview-2818619507)
ACK fc60337733a9dffaa42e08fcbff0ab24b5f679a4
i looked over the changes and did some spot checks and found nothing that looks malicious or appears out of the ordinary.
💬 ryanofsky commented on pull request "[DRAFT] ipc: add windows support":
(https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2854925140)
Updated f215e742045401ab386f0cd67d06b358558ecf89 -> 5f272b7cc3b72fe4a59f590825b68dc2c342baca ([`pr/ipc-win.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.5) -> [`pr/ipc-win.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-win.5..pr/ipc-win.6)) with more fixes.
With these fixes, IPC is fully working on windows: both creating child processes and communicating over socket pairs, and listening and making con
...
(https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2854925140)
Updated f215e742045401ab386f0cd67d06b358558ecf89 -> 5f272b7cc3b72fe4a59f590825b68dc2c342baca ([`pr/ipc-win.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.5) -> [`pr/ipc-win.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-win.5..pr/ipc-win.6)) with more fixes.
With these fixes, IPC is fully working on windows: both creating child processes and communicating over socket pairs, and listening and making con
...
🤔 rkrux reviewed a pull request: "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor"
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2818659786)
> The importdescriptors RPC sets the range for all non-ranged descriptors to [0,1] in every request so this error cannot be triggered using the RPC.
I checked that the `importdescriptors` adds a 1 to range end if a range is given in the request.
> that this error may be unexpectedly triggered when " experimenting with non-ranged descriptors in custom wallet workflows.
The issue seems quite peculiar in that it highlights an issue within an internal function instead of the full functional
...
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2818659786)
> The importdescriptors RPC sets the range for all non-ranged descriptors to [0,1] in every request so this error cannot be triggered using the RPC.
I checked that the `importdescriptors` adds a 1 to range end if a range is given in the request.
> that this error may be unexpectedly triggered when " experimenting with non-ranged descriptors in custom wallet workflows.
The issue seems quite peculiar in that it highlights an issue within an internal function instead of the full functional
...
💬 sr-gi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2854968169)
I think the limits have proven ineffective and would be in favour of dropping them. However, I also acknowledge this has created friction amongst users who think they should have a saying in what their node configuration should be. While I think this is a placebo at best and most likely a shot in the foot, I'd be in favour of increasing the limit but not removing the configuration option.
We can revisit this in the future, backed up with data on how the increase has affected the inclusion of
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2854968169)
I think the limits have proven ineffective and would be in favour of dropping them. However, I also acknowledge this has created friction amongst users who think they should have a saying in what their node configuration should be. While I think this is a placebo at best and most likely a shot in the foot, I'd be in favour of increasing the limit but not removing the configuration option.
We can revisit this in the future, backed up with data on how the increase has affected the inclusion of
...
🤔 marcofleon reviewed a pull request: "test: added fuzz coverage for consensus/merkle.cpp"
(https://github.com/bitcoin/bitcoin/pull/32243#pullrequestreview-2818609635)
Good stuff, nice to have new coverage.
I think it'd be worth it to add an oracle in the test where you reconstruct the merkle root with the path you get from `TransactionMerklePath` and verify that it matches the root from the earlier `ComputeMerkleRoot` call.
(https://github.com/bitcoin/bitcoin/pull/32243#pullrequestreview-2818609635)
Good stuff, nice to have new coverage.
I think it'd be worth it to add an oracle in the test where you reconstruct the merkle root with the path you get from `TransactionMerklePath` and verify that it matches the root from the earlier `ComputeMerkleRoot` call.
💬 marcofleon commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2075711453)
Meant to remove this line?
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2075711453)
Meant to remove this line?
💬 marcofleon commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2075666086)
This should be 0 to `num_txs - 1` no? Just based on the [coverage](https://marcofleon.github.io/coverage/merkle/coverage/root/bitcoin/src/consensus/merkle.cpp.html) it seems the fuzzer is choosing `num_txs` which means `matchh` won't be set to true here:
https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/src/consensus/merkle.cpp#L111
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2075666086)
This should be 0 to `num_txs - 1` no? Just based on the [coverage](https://marcofleon.github.io/coverage/merkle/coverage/root/bitcoin/src/consensus/merkle.cpp.html) it seems the fuzzer is choosing `num_txs` which means `matchh` won't be set to true here:
https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/src/consensus/merkle.cpp#L111