💬 glozow commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2877848547)
reACK 8673e8f0191
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2877848547)
reACK 8673e8f0191
🤔 glozow reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2838031475)
reACK 8673e8f0191
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2838031475)
reACK 8673e8f0191
🚀 glozow merged a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444)
(https://github.com/bitcoin/bitcoin/pull/31444)
🚀 glozow merged a pull request: "test: Fix feature_pruning test after nTime typo fix"
(https://github.com/bitcoin/bitcoin/pull/32312)
(https://github.com/bitcoin/bitcoin/pull/32312)
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-2877875187)
Now ready for review, with #31444 merged.
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-2877875187)
Now ready for review, with #31444 merged.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2087588925)
As I understand it it returns different values across restarts (which suffices here) but I may be wrong. The doc is confusing. Anyway, your solution doesn't hurt.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2087588925)
As I understand it it returns different values across restarts (which suffices here) but I may be wrong. The doc is confusing. Anyway, your solution doesn't hurt.
🤔 musaHaruna reviewed a pull request: "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`"
(https://github.com/bitcoin/bitcoin/pull/32432#pullrequestreview-2838067718)
tACK [2fee8f4z](https://github.com/bitcoin/bitcoin/pull/32432/commits/2fee8f4886e1421099dfb8ba959b9d0c7e4b0aa8)
Compiled Successfully without errors
Test the code functionality using:
`build/bin/bitcoin-cli -regtest help getnewaddress `
Result:
`Arguments:
.....
2. address_type (string, optional, default=set by -addresstype) The address type to use. Options are "legacy", "p2sh-segwit", "bech32", "bech32m".`
.....
(https://github.com/bitcoin/bitcoin/pull/32432#pullrequestreview-2838067718)
tACK [2fee8f4z](https://github.com/bitcoin/bitcoin/pull/32432/commits/2fee8f4886e1421099dfb8ba959b9d0c7e4b0aa8)
Compiled Successfully without errors
Test the code functionality using:
`build/bin/bitcoin-cli -regtest help getnewaddress `
Result:
`Arguments:
.....
2. address_type (string, optional, default=set by -addresstype) The address type to use. Options are "legacy", "p2sh-segwit", "bech32", "bech32m".`
.....
🤔 glozow reviewed a pull request: "mining: rename gbt_force and gbt_force_name"
(https://github.com/bitcoin/bitcoin/pull/32386#pullrequestreview-2838070577)
ACK 0750249289c092fc8e2e29669fec73a58b873767, seems sensible
(https://github.com/bitcoin/bitcoin/pull/32386#pullrequestreview-2838070577)
ACK 0750249289c092fc8e2e29669fec73a58b873767, seems sensible
🚀 glozow merged a pull request: "mining: rename gbt_force and gbt_force_name"
(https://github.com/bitcoin/bitcoin/pull/32386)
(https://github.com/bitcoin/bitcoin/pull/32386)
💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2087601815)
Yeah, I had hoped for the same :(
The problem here (as usual with these annotations) is aliasing.
As a simplified example:
```c++
CCheckQueueControl<CScriptCheck> control(m_chainman.GetCheckQueue());
CCheckQueueControl<CScriptCheck> control2(m_chainman.GetCheckQueue());
```
Here, clang has no idea that `&control.m_lock.m_control_mutex` == `&control2.m_lock.m_control_mutex`.
The only alternative, really, would be something like:
```diff
diff --git a/src/checkqueue.h b/src/chec
...
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2087601815)
Yeah, I had hoped for the same :(
The problem here (as usual with these annotations) is aliasing.
As a simplified example:
```c++
CCheckQueueControl<CScriptCheck> control(m_chainman.GetCheckQueue());
CCheckQueueControl<CScriptCheck> control2(m_chainman.GetCheckQueue());
```
Here, clang has no idea that `&control.m_lock.m_control_mutex` == `&control2.m_lock.m_control_mutex`.
The only alternative, really, would be something like:
```diff
diff --git a/src/checkqueue.h b/src/chec
...
🤔 glozow reviewed a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080#pullrequestreview-2838089123)
ACK 1e0de7a6ba926487c8a075856b74af2a3a0eb8ef
(https://github.com/bitcoin/bitcoin/pull/31080#pullrequestreview-2838089123)
ACK 1e0de7a6ba926487c8a075856b74af2a3a0eb8ef
✅ glozow closed an issue: "Bitcoin is returning higher fees for 36 block window than 2 block window (on testnet)"
(https://github.com/bitcoin/bitcoin/issues/11800)
(https://github.com/bitcoin/bitcoin/issues/11800)
🚀 glozow merged a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080)
(https://github.com/bitcoin/bitcoin/pull/31080)
💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2087617462)
Done.
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2087617462)
Done.
💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2877918269)
Updated to address @TheCharlatan's nit.
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2877918269)
Updated to address @TheCharlatan's nit.
🤔 stringintech reviewed a pull request: "kernel: Separate UTXO set access from validation functions"
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2837977865)
I had a few more comments (sorry for the scattered feedback; didn't have the chance to review everything earlier).
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2837977865)
I had a few more comments (sorry for the scattered feedback; didn't have the chance to review everything earlier).
💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087608225)
In commit 5908680, the description mentions: "By returning early in case of a BIP30 validation failure, an additional failure check for an invalid block reward in case of its failure is left out."
To validate my understanding: there are also other checks in `ConnectBlock` before the `!state.IsValid()` check that we may have skipped because of the early return when `SpendBlock` fails - including transaction input validation, script verification, fee range checks, ...; Is that correct?
It ma
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087608225)
In commit 5908680, the description mentions: "By returning early in case of a BIP30 validation failure, an additional failure check for an invalid block reward in case of its failure is left out."
To validate my understanding: there are also other checks in `ConnectBlock` before the `!state.IsValid()` check that we may have skipped because of the early return when `SpendBlock` fails - including transaction input validation, script verification, fee range checks, ...; Is that correct?
It ma
...
💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087539901)
Looks like we have an early return in `GetTransactionSigOpCost` for coinbase tx and no use for `coins` span; so maybe we could just pass an empty span here?
```suggestion
nSigOpsCost += GetTransactionSigOpCost<const Coin>(tx, {}, flags);
```
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087539901)
Looks like we have an early return in `GetTransactionSigOpCost` for coinbase tx and no use for `coins` span; so maybe we could just pass an empty span here?
```suggestion
nSigOpsCost += GetTransactionSigOpCost<const Coin>(tx, {}, flags);
```
💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087577718)
Do we need to update the [documentation](https://github.com/TheCharlatan/bitcoin/blob/16a695fbff4a92eba3eb72ec6aa766e71c52f773/src/consensus/tx_verify.h#L24:L31) for `CheckTxInputs` now that the input existence check is removed from it?
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087577718)
Do we need to update the [documentation](https://github.com/TheCharlatan/bitcoin/blob/16a695fbff4a92eba3eb72ec6aa766e71c52f773/src/consensus/tx_verify.h#L24:L31) for `CheckTxInputs` now that the input existence check is removed from it?
📝 maflcko opened a pull request: "fuzz: Properly setup wallet in wallet_fees target"
(https://github.com/bitcoin/bitcoin/pull/32488)
`g_wallet_ptr` is destructed after the `testing_setup`. This is not supported and will lead to issues such as https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2863875857 or https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2855259932.
This could be fixed by fixing the initialization order.
However, the global wallet is also modified in the fuzz target, which is bad fuzzing practise.
So instead fix it by constructing a fresh wallet for each fuzz iteration.
(https://github.com/bitcoin/bitcoin/pull/32488)
`g_wallet_ptr` is destructed after the `testing_setup`. This is not supported and will lead to issues such as https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2863875857 or https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2855259932.
This could be fixed by fixing the initialization order.
However, the global wallet is also modified in the fuzz target, which is bad fuzzing practise.
So instead fix it by constructing a fresh wallet for each fuzz iteration.