💬 luke-jr commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2591184498)
Can we predict the memory usage spike size? Presumably as we flush, that releases memory, which allows for a larger and larger batch size?
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2591184498)
Can we predict the memory usage spike size? Presumably as we flush, that releases memory, which allows for a larger and larger batch size?
👍 ryanofsky approved a pull request: "test: Add test for rpcwhitelistdefault"
(https://github.com/bitcoin/bitcoin/pull/29858#pullrequestreview-2551114829)
Code review ACK cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b. Left a few more suggestions to consider, but cleanups look good and I think this is pretty easy to understand now.
(https://github.com/bitcoin/bitcoin/pull/29858#pullrequestreview-2551114829)
Code review ACK cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b. Left a few more suggestions to consider, but cleanups look good and I think this is pretty easy to understand now.
💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1915663066)
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_r1915663066)
In commit "test: Add test for rpcwhitelistdefault" (cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b)
Note: since test was updated "user3" should now be "strangedude6"
💬 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".
>
>
...