💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1915666383)
In commit "test: Add test for rpcwhitelistdefault" (cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b)
It's a big confusing calling this test rpcwhitelistdefault_1 when nothing in this test actually depends on `rpcwhitelistdefault` value, and the test will pass whether it is 0 or 1. Would suggest renaming the method and calling it with both values to make sure behavior is unaffected. Would suggest:
```diff
--- a/test/functional/rpc_whitelist.py
+++ b/test/functional/rpc_whitelist.py
@@ -94,6 +94
...
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1915666383)
In commit "test: Add test for rpcwhitelistdefault" (cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b)
It's a big confusing calling this test rpcwhitelistdefault_1 when nothing in this test actually depends on `rpcwhitelistdefault` value, and the test will pass whether it is 0 or 1. Would suggest renaming the method and calling it with both values to make sure behavior is unaffected. Would suggest:
```diff
--- a/test/functional/rpc_whitelist.py
+++ b/test/functional/rpc_whitelist.py
@@ -94,6 +94
...
💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1915662983)
In commit "test: Add test for rpcwhitelistdefault" (cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b)
Note: since test was updated "user3" should now be "strangedude6"
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1915662983)
In commit "test: Add test for rpcwhitelistdefault" (cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b)
Note: since test was updated "user3" should now be "strangedude6"
👍 theuni approved a pull request: "refactor: Avoid UB in SHA3_256::Write"
(https://github.com/bitcoin/bitcoin/pull/31655#pullrequestreview-2551130478)
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1
(https://github.com/bitcoin/bitcoin/pull/31655#pullrequestreview-2551130478)
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1
💬 mzumsande commented on pull request "wallet: fix rescanning inconsistency":
(https://github.com/bitcoin/bitcoin/pull/31629#issuecomment-2591235952)
> I think adjusting m_last_processed_block instead would be the right fix.
I wanted to do that initially, but I think it's incorrect: If we set `m_last_processed_block` for block 1 until block N within the rescan, and after the rescan has finished we process the outstanding `blockConnected` notification for block 1, `m_last_processed_block` will be set backwards to the the wrong block, and the wallet will be in a inconsistent state until all other outstanding notifications until block N are p
...
(https://github.com/bitcoin/bitcoin/pull/31629#issuecomment-2591235952)
> I think adjusting m_last_processed_block instead would be the right fix.
I wanted to do that initially, but I think it's incorrect: If we set `m_last_processed_block` for block 1 until block N within the rescan, and after the rescan has finished we process the outstanding `blockConnected` notification for block 1, `m_last_processed_block` will be set backwards to the the wrong block, and the wallet will be in a inconsistent state until all other outstanding notifications until block N are p
...
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915697166)
Ah, of course. The actual problem is that we are shifting even out of the widened type's range, so it wraps around. Not sure if there is a nice solution to that without re-implementing the logic we are trying to test.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915697166)
Ah, of course. The actual problem is that we are shifting even out of the widened type's range, so it wraps around. Not sure if there is a nice solution to that without re-implementing the logic we are trying to test.
🤔 ismaelsadeeq reviewed a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2551158656)
Concept ACK
I think the added feature here is useful to miners!
The consensus changes involve splitting `CheckProofOfWorkImpl` into two functions:
1. `DeriveTarget`, which returns the proof-of-work target given a compact target.
2. `CheckProofOfWorkImpl`, which performs its normal check to verify whether a block hash satisfies the proof-of-work requirements.
This enables the usage of `DeriveTarget` in other places.
The large diff is due to an alternate mainnet chain data used i
...
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2551158656)
Concept ACK
I think the added feature here is useful to miners!
The consensus changes involve splitting `CheckProofOfWorkImpl` into two functions:
1. `DeriveTarget`, which returns the proof-of-work target given a compact target.
2. `CheckProofOfWorkImpl`, which performs its normal check to verify whether a block hash satisfies the proof-of-work requirements.
This enables the usage of `DeriveTarget` in other places.
The large diff is due to an alternate mainnet chain data used i
...
💬 ismaelsadeeq commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1915695379)
@Sjors what do you mean by "if found now" here and the other places?
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1915695379)
@Sjors what do you mean by "if found now" here and the other places?
💬 luke-jr commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2591252240)
>All checkpoints and checkpoint logic are removed in a single commit, to make it easy to revert if necessary.
It's easy to revert an entire merge, so feel free to split it up if it makes sense to...
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2591252240)
>All checkpoints and checkpoint logic are removed in a single commit, to make it easy to revert if necessary.
It's easy to revert an entire merge, so feel free to split it up if it makes sense to...
💬 brunoerg commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#discussion_r1915719223)
In 74fa29e12e379d7be8ad2dd261ee68efaf7a9e07: `privkeys` can be removed, it's unused.
(https://github.com/bitcoin/bitcoin/pull/31378#discussion_r1915719223)
In 74fa29e12e379d7be8ad2dd261ee68efaf7a9e07: `privkeys` can be removed, it's unused.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2591295705)
Thanks for your review @Sjors and recent insight from @0xB10C
I've force pushed from dba211a9ceafc57a953efc757adfe09c0f3fdb14 to ad1bc03245181b00a25ea0182373eddae1c151e1 see [diff](https://github.com/bitcoin/bitcoin/compare/dba211a9ceafc57a953efc757adfe09c0f3fdb14..9475218d1ff5a2cbe8b682252715b10d489c22a5)
Changes
1. Renamed `-coinbasereservedweight` to `-blockreservedweight`
2. Clarified that the reservation is also for block header data
3. Prevent startup when user provided a `-blockr
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2591295705)
Thanks for your review @Sjors and recent insight from @0xB10C
I've force pushed from dba211a9ceafc57a953efc757adfe09c0f3fdb14 to ad1bc03245181b00a25ea0182373eddae1c151e1 see [diff](https://github.com/bitcoin/bitcoin/compare/dba211a9ceafc57a953efc757adfe09c0f3fdb14..9475218d1ff5a2cbe8b682252715b10d489c22a5)
Changes
1. Renamed `-coinbasereservedweight` to `-blockreservedweight`
2. Clarified that the reservation is also for block header data
3. Prevent startup when user provided a `-blockr
...
📝 davidgumberg opened a pull request: "ci: Supply `--platform` argument to `docker build`."
(https://github.com/bitcoin/bitcoin/pull/31657)
I ran into this issue when following the instructions in `ci/README.md` for running CI locally.
Newer versions of docker require a `--platform` argument when building from a platform-specific image that differs from the host platform, I'm not sure when this change took place, but trying to build any of the cross-platform CI images on Docker 27.5.0 fails in the following manner:
```console
$ # From ci/README.md
$ env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'FILE_ENV="./ci/test/00_
...
(https://github.com/bitcoin/bitcoin/pull/31657)
I ran into this issue when following the instructions in `ci/README.md` for running CI locally.
Newer versions of docker require a `--platform` argument when building from a platform-specific image that differs from the host platform, I'm not sure when this change took place, but trying to build any of the cross-platform CI images on Docker 27.5.0 fails in the following manner:
```console
$ # From ci/README.md
$ env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'FILE_ENV="./ci/test/00_
...
💬 brunoerg commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1915747545)
No objections on it but maybe you could update the documentation, it says: "This involves stripping sigscripts from each tx and ensuring txid is consistent." but you're also stripping the script witness.
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1915747545)
No objections on it but maybe you could update the documentation, it says: "This involves stripping sigscripts from each tx and ensuring txid is consistent." but you're also stripping the script witness.
💬 luke-jr commented on pull request "depends: Qt 5.15.15":
(https://github.com/bitcoin/bitcoin/pull/30774#issuecomment-2591376843)
Explicit intention to open source 5.15.16: https://lists.qt-project.org/pipermail/announce/2024-November/000526.html
Probably should reopen this for backports at least?
(https://github.com/bitcoin/bitcoin/pull/30774#issuecomment-2591376843)
Explicit intention to open source 5.15.16: https://lists.qt-project.org/pipermail/announce/2024-November/000526.html
Probably should reopen this for backports at least?
👋 EthanHeilman's pull request is ready for review: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640)
(https://github.com/bitcoin/bitcoin/pull/31640)
💬 0xBEEFCAF3 commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r1915819609)
Super nit: empty space on this line. Not sure if the linter even picks up on this
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r1915819609)
Super nit: empty space on this line. Not sure if the linter even picks up on this
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r1915820425)
Fixed! Thanks
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r1915820425)
Fixed! Thanks
💬 RolloTomasiii commented on issue "Error unlocking wallet: some keys decrypt but not all. your wallet file may be corrupt. ":
(https://github.com/bitcoin/bitcoin/issues/29789#issuecomment-2591482979)
> ### Is there an existing issue for this?
> * [x] I have searched the existing issues
>
> ### Current behaviour
> My personal computer crashed and I've already imported "wallet.dat" in the latest Bitcoin Core v26.0.0. It takes time to synchronize the wallet that was about 6 days.
>
> Anyhow, now my wallet is updated and I can see the correct balance of my Bitcoin in the "Overview section". I have all the needed information which I took as a backup such as "passphrases" and "wallet.dat".
>
>
...
(https://github.com/bitcoin/bitcoin/issues/29789#issuecomment-2591482979)
> ### Is there an existing issue for this?
> * [x] I have searched the existing issues
>
> ### Current behaviour
> My personal computer crashed and I've already imported "wallet.dat" in the latest Bitcoin Core v26.0.0. It takes time to synchronize the wallet that was about 6 days.
>
> Anyhow, now my wallet is updated and I can see the correct balance of my Bitcoin in the "Overview section". I have all the needed information which I took as a backup such as "passphrases" and "wallet.dat".
>
>
...
💬 davidgumberg commented on pull request "ci: Bump centos stream 10":
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2591506347)
Concept ACK
The CentOS Stream 9 CI task as-is uses the core python3 packages which are `3.9`, below the minimum `3.10` and results in a syntax error any time `test/functional/combine_logs.py` is invoked since [it uses](https://github.com/bitcoin/bitcoin/commit/c9fb38a590e3961e68e942d71f202e357466d15f) a `match` statement.
It seems that CMake has been complaining in every single centos CI run[^1] since the minimum version was bumped to [3.10](https://github.com/bitcoin/bitcoin/pull/30527/)
...
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2591506347)
Concept ACK
The CentOS Stream 9 CI task as-is uses the core python3 packages which are `3.9`, below the minimum `3.10` and results in a syntax error any time `test/functional/combine_logs.py` is invoked since [it uses](https://github.com/bitcoin/bitcoin/commit/c9fb38a590e3961e68e942d71f202e357466d15f) a `match` statement.
It seems that CMake has been complaining in every single centos CI run[^1] since the minimum version was bumped to [3.10](https://github.com/bitcoin/bitcoin/pull/30527/)
...
💬 TheBlueMatt commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2591522083)
> 1. reduces (tail) latency when requesting a block. In most cases it won't matter, but immediately after finding a new block there is potentially a lot of cs_main contention with other peers sending us copies of the block header or peers requesting the block's transactions. Even ignoring the on-block stuff, you don't want to find yourself waiting for some relatively large transaction to fetch a bunch of UTXO entries off disk to get into the mempool to build your new block!
An interface to re
...
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2591522083)
> 1. reduces (tail) latency when requesting a block. In most cases it won't matter, but immediately after finding a new block there is potentially a lot of cs_main contention with other peers sending us copies of the block header or peers requesting the block's transactions. Even ignoring the on-block stuff, you don't want to find yourself waiting for some relatively large transaction to fetch a bunch of UTXO entries off disk to get into the mempool to build your new block!
An interface to re
...
💬 davidgumberg commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2591535568)
ACK https://github.com/bitcoin/bitcoin/pull/31651/commits/fabefd9915802d53d8c83ae66e5cb259196d9422
Ran locally and all tests passed.
```bash
env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'FILE_ENV="./ci/test/00_setup_env_native_centos.sh" ./ci/test_run_all.sh'
```
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2591535568)
ACK https://github.com/bitcoin/bitcoin/pull/31651/commits/fabefd9915802d53d8c83ae66e5cb259196d9422
Ran locally and all tests passed.
```bash
env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'FILE_ENV="./ci/test/00_setup_env_native_centos.sh" ./ci/test_run_all.sh'
```
📝 theStack opened a pull request: "test: p2p: fix sending of manual INVs in tx download test"
(https://github.com/bitcoin/bitcoin/pull/31658)
The `test_inv_block` sub-test in p2p_tx_download.py has a subtle bug: the manual msg_inv announcements from peers currently have no effect, since they don't match the wtxidrelay setting (=true by default for `P2PInterface` instances) and are hence ignored by the nodes (since 2d282e0c / PR #18044):
https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/src/net_processing.cpp#L3904-L3911
Though the test still passes on master, it does so without the intended scenari
...
(https://github.com/bitcoin/bitcoin/pull/31658)
The `test_inv_block` sub-test in p2p_tx_download.py has a subtle bug: the manual msg_inv announcements from peers currently have no effect, since they don't match the wtxidrelay setting (=true by default for `P2PInterface` instances) and are hence ignored by the nodes (since 2d282e0c / PR #18044):
https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/src/net_processing.cpp#L3904-L3911
Though the test still passes on master, it does so without the intended scenari
...