🤔 pablomartin4btc reviewed a pull request: "wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance"
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2915057715)
Concept ACK
There are still functional test references to the fields removed in the first commit (also in `wallet_multiwallet.py`, `wallet_reorgsrestore.py`). Should references in the wallet interface be updated as well?
This would need release notes at some point, no?
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2915057715)
Concept ACK
There are still functional test references to the fields removed in the first commit (also in `wallet_multiwallet.py`, `wallet_reorgsrestore.py`). Should references in the wallet interface be updated as well?
This would need release notes at some point, no?
🤔 mzumsande reviewed a pull request: "index: move disk read lookups to base class"
(https://github.com/bitcoin/bitcoin/pull/32694#pullrequestreview-2914928430)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32694#pullrequestreview-2914928430)
Concept ACK
💬 mzumsande commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138697822)
6f1392cc42cde638773f2b697d7d2c58abcdc860:
As a result of this commit, `CopyHeightIndexToHashIndex` (which doesn't have any other callers) could be simpler: No nee to pass two heights, no need for a loop if it only copies one element.
Besides, I wonder if the code duplication (it's identical code in `coinstatsindex` and `blockfilterindex`) could be resolved.
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138697822)
6f1392cc42cde638773f2b697d7d2c58abcdc860:
As a result of this commit, `CopyHeightIndexToHashIndex` (which doesn't have any other callers) could be simpler: No nee to pass two heights, no need for a loop if it only copies one element.
Besides, I wonder if the code duplication (it's identical code in `coinstatsindex` and `blockfilterindex`) could be resolved.
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138805235)
Nice catch! I've added a commit that always performs the check for SimpleCandidateFinder (even if non-optimal).
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138805235)
Nice catch! I've added a commit that always performs the check for SimpleCandidateFinder (even if non-optimal).
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138805358)
Indeed.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138805358)
Indeed.
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138806088)
The chunking won't necessarily match exactly, but the number of elements in the chunking must match if both are optimal.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138806088)
The chunking won't necessarily match exactly, but the number of elements in the chunking must match if both are optimal.
💬 achow101 commented on issue "`signrawtransactionwithkey` doesn't work with non segwit p2sh scripts":
(https://github.com/bitcoin/bitcoin/issues/32722#issuecomment-2960680889)
Your test has a bug:
```
script_b = script_to_p2sh_script(redeem_script)
address = script_to_p2sh(script_b)
```
`address` is computed incorrectly. `script_to-p2sh` is like `script_to_p2sh_script` in that it takes the redeem script. When you pass it `script_b`, what you're really creating is a `p2sh(p2sh())` which is invalid. I believe if you modify your test to try to broadcast the transaction, it would fail to validate.
(https://github.com/bitcoin/bitcoin/issues/32722#issuecomment-2960680889)
Your test has a bug:
```
script_b = script_to_p2sh_script(redeem_script)
address = script_to_p2sh(script_b)
```
`address` is computed incorrectly. `script_to-p2sh` is like `script_to_p2sh_script` in that it takes the redeem script. When you pass it `script_b`, what you're really creating is a `p2sh(p2sh())` which is invalid. I believe if you modify your test to try to broadcast the transaction, it would fail to validate.
🤔 w0xlt reviewed a pull request: "wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance"
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2915146785)
ACK https://github.com/bitcoin/bitcoin/pull/32721/commits/19a2f0ca09b9b56485d21e309486906c26e8afcf
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2915146785)
ACK https://github.com/bitcoin/bitcoin/pull/32721/commits/19a2f0ca09b9b56485d21e309486906c26e8afcf
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138844396)
Huh, I hit fuzz failures with this if I let it run for a while. Weird, will investigate.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2138844396)
Huh, I hit fuzz failures with this if I let it run for a while. Weird, will investigate.
📝 theuni opened a pull request: "Refactor: Redefine CTransaction equality to include witness data"
(https://github.com/bitcoin/bitcoin/pull/32723)
I stumbled upon the `CTransaction` comparison operators while refactoring some nearby code. I found it surprising and not at all obvious that two transactions would test equal even if their witness data differed. It seems like an unnecessary potential footgun. Fix that by comparing against wtxid rather than txid.
Outside of tests, there were only 3 users of these functions in the code-base:
- Its use in the mempool has been replaced with an explicit txid comparison, as that's a tighter const
...
(https://github.com/bitcoin/bitcoin/pull/32723)
I stumbled upon the `CTransaction` comparison operators while refactoring some nearby code. I found it surprising and not at all obvious that two transactions would test equal even if their witness data differed. It seems like an unnecessary potential footgun. Fix that by comparing against wtxid rather than txid.
Outside of tests, there were only 3 users of these functions in the code-base:
- Its use in the mempool has been replaced with an explicit txid comparison, as that's a tighter const
...
💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138853635)
This reminds me to my own [comment](https://github.com/bitcoin/bitcoin/pull/24230/commits/707ff84981d7f61d467093cb0aec7eee55276c52#r1307972744) hehe. Will do it here.
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138853635)
This reminds me to my own [comment](https://github.com/bitcoin/bitcoin/pull/24230/commits/707ff84981d7f61d467093cb0aec7eee55276c52#r1307972744) hehe. Will do it here.
💬 theuni commented on pull request "Refactor: Redefine CTransaction equality to include witness data":
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-2960706099)
Ping @achow101 and @glozow to check my assumptions about the wallet/mempool uses.
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-2960706099)
Ping @achow101 and @glozow to check my assumptions about the wallet/mempool uses.
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2960748073)
As @LarryRuane noticed, the new address-diff test sometimes disagrees with `MallocUsage` on macOS and on sanitizer builds.
I had to read a bit to understand why - I never needed the gory details until now. 🙂
It seem to me that macOS's nano allocator, and the allocators used by TSan/ASan, store per-chunk metadata in shadow tables rather than inline with the user block. For example, this might be related to what we're seeing: https://github.com/aosm/libmalloc/blob/master/src/nano_malloc.c#L
...
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2960748073)
As @LarryRuane noticed, the new address-diff test sometimes disagrees with `MallocUsage` on macOS and on sanitizer builds.
I had to read a bit to understand why - I never needed the gory details until now. 🙂
It seem to me that macOS's nano allocator, and the allocators used by TSan/ASan, store per-chunk metadata in shadow tables rather than inline with the user block. For example, this might be related to what we're seeing: https://github.com/aosm/libmalloc/blob/master/src/nano_malloc.c#L
...
💬 douglaz commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960761883)
> Also - referring to those who disagree with you as "twitter bots" or - as @glozow continually does (even in formal statements) that we're just using LLMs is not just frustrating and disrespectful to those who disagree with you, but makes you all look like elitist snobs.
Both sides have engaged in loaded language; for instance, calling consensus-valid transactions “spam.” I have likewise seen disrespect from the pro-filter camp, including personal attacks on Core developers and on anyone w
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960761883)
> Also - referring to those who disagree with you as "twitter bots" or - as @glozow continually does (even in formal statements) that we're just using LLMs is not just frustrating and disrespectful to those who disagree with you, but makes you all look like elitist snobs.
Both sides have engaged in loaded language; for instance, calling consensus-valid transactions “spam.” I have likewise seen disrespect from the pro-filter camp, including personal attacks on Core developers and on anyone w
...
💬 negatratoron commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960786220)
I'm not even sure that data carrier transactions are spam in the first place. The blockchain is an immutable structure that can store arbitrary data trustlessly - maybe block space is the product. and bitcoins are the tokens you can use to buy block space. Maybe thinking of bitcoin as a pure "currency" in the first place was wrong, and it's actually a tool for recording information immutably in the public domain.
You can architect the blockchain to be able to store data at some rate, for so
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2960786220)
I'm not even sure that data carrier transactions are spam in the first place. The blockchain is an immutable structure that can store arbitrary data trustlessly - maybe block space is the product. and bitcoins are the tokens you can use to buy block space. Maybe thinking of bitcoin as a pure "currency" in the first place was wrong, and it's actually a tool for recording information immutably in the public domain.
You can architect the blockchain to be able to store data at some rate, for so
...
💬 achow101 commented on pull request "Refactor: Redefine CTransaction equality to include witness data":
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-2960797103)
`CWalletTx::IsEquivalentTo` needs to consider malleated txs as equivalent, so the original behavior of ignoring the scriptSigs and scriptWitness is intended.
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-2960797103)
`CWalletTx::IsEquivalentTo` needs to consider malleated txs as equivalent, so the original behavior of ignoring the scriptSigs and scriptWitness is intended.
💬 w0xlt commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2138916560)
Maybe a test in `src/test/util_tests.cpp` in `BOOST_AUTO_TEST_CASE(test_script_parsing)` would be good for consistency's sake.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2138916560)
Maybe a test in `src/test/util_tests.cpp` in `BOOST_AUTO_TEST_CASE(test_script_parsing)` would be good for consistency's sake.
💬 theStack commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#discussion_r2138926469)
Good point, decided to squash the two mentioned commits to get rid of the repeated renaming. I.e. both `.rehash()` and `.hash` calls/accesses of CTransaction instances are replaced with `.txid_hex` now in a single commit (ce83924237127fe6d32b63c362b57789e4628954).
(https://github.com/bitcoin/bitcoin/pull/32421#discussion_r2138926469)
Good point, decided to squash the two mentioned commits to get rid of the repeated renaming. I.e. both `.rehash()` and `.hash` calls/accesses of CTransaction instances are replaced with `.txid_hex` now in a single commit (ce83924237127fe6d32b63c362b57789e4628954).
🤔 w0xlt reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2915269775)
Perhaps the description of the commits `script/parsing: Allow Const to not skip the found constant ` (https://github.com/bitcoin/bitcoin/pull/31244/commits/4b135f80d03c92fb522d1dcfd7d697aa0a4af626) and `util/string: Allow Split to include the separator` (https://github.com/bitcoin/bitcoin/pull/31244/commits/b89a937225ed217556b38e000ee5d1bf0b5952aa) could explain why these changes are necessary for the musig2 descriptor.
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2915269775)
Perhaps the description of the commits `script/parsing: Allow Const to not skip the found constant ` (https://github.com/bitcoin/bitcoin/pull/31244/commits/4b135f80d03c92fb522d1dcfd7d697aa0a4af626) and `util/string: Allow Split to include the separator` (https://github.com/bitcoin/bitcoin/pull/31244/commits/b89a937225ed217556b38e000ee5d1bf0b5952aa) could explain why these changes are necessary for the musig2 descriptor.
🚀 achow101 merged a pull request: "ci: Rewrite test-each-commit as py script"
(https://github.com/bitcoin/bitcoin/pull/32680)
(https://github.com/bitcoin/bitcoin/pull/32680)