💬 pinheadmz commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2578808220)
@luke-jr can you rephrase your comment in a friendlier way please
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2578808220)
@luke-jr can you rephrase your comment in a friendlier way please
💬 mzumsande commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1907957838)
I agree that it should be possible to drop the check.
This is a off-topic here, but maybe the entire `NeedsRedownload()` check could be removed at some point, or simplified to just check block one after segwit, instead over iterating over 400k indexes each startup? Segwit activation is now more than 8 years in the past after all...
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1907957838)
I agree that it should be possible to drop the check.
This is a off-topic here, but maybe the entire `NeedsRedownload()` check could be removed at some point, or simplified to just check block one after segwit, instead over iterating over 400k indexes each startup? Segwit activation is now more than 8 years in the past after all...
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2578874634)
I have simplified the eviction interface. Instead of getting a `TxGraph::Evictor` object through `TxGraph::GetEvictor()`, there is now a simpler `TxGraph::GetWorstMainChunk()` function, which is a pure inspector.
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2578874634)
I have simplified the eviction interface. Instead of getting a `TxGraph::Evictor` object through `TxGraph::GetEvictor()`, there is now a simpler `TxGraph::GetWorstMainChunk()` function, which is a pure inspector.
💬 davidgumberg commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2578881602)
Concept ACK
I second the suggestion by @tdb3 [above](https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1885462353) to also check `-walletdir`, although I have looked and have not been able to find similar reports of corruption with SQLite and exFAT drives on macOS.
To add a little data pointing in the direction of this bug only existing on macOS, I have synced nodes with datadirs on exFAT drives over both SATA and USB on Fedora 40 and have a long-running bitcoind node that has been
...
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2578881602)
Concept ACK
I second the suggestion by @tdb3 [above](https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1885462353) to also check `-walletdir`, although I have looked and have not been able to find similar reports of corruption with SQLite and exFAT drives on macOS.
To add a little data pointing in the direction of this bug only existing on macOS, I have synced nodes with datadirs on exFAT drives over both SATA and USB on Fedora 40 and have a long-running bitcoind node that has been
...
💬 luke-jr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907984056)
This should remain logically separate from consensus constant.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907984056)
This should remain logically separate from consensus constant.
💬 luke-jr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907983031)
This changes the meaning of `getmininginfo` fields unexpectedly, and forces us to add in the coinbase reserved size over and over. Seems like it would be better to include it here once.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907983031)
This changes the meaning of `getmininginfo` fields unexpectedly, and forces us to add in the coinbase reserved size over and over. Seems like it would be better to include it here once.
💬 0xB10C commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2578892122)
> I tried testing the asan job, via `time MAKEJOBS="-j17" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh`, on a Fedora dev box, and it fails with the following output.
I guess checking that were in a docker container (that hopefully doesn't have other services that does anything network related) is the only option here. Only fail in docker, otherwise just print, if possible.
Possibly by checking that `/proc/1/cgroup` exists and this is not empty with `cat /proc/1/cgroup
...
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2578892122)
> I tried testing the asan job, via `time MAKEJOBS="-j17" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh`, on a Fedora dev box, and it fails with the following output.
I guess checking that were in a docker container (that hopefully doesn't have other services that does anything network related) is the only option here. Only fail in docker, otherwise just print, if possible.
Possibly by checking that `/proc/1/cgroup` exists and this is not empty with `cat /proc/1/cgroup
...
💬 davidgumberg commented on issue "Data corruption on MacOS when using exFAT datadir or blocksdir":
(https://github.com/bitcoin/bitcoin/issues/31454#issuecomment-2578940904)
https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2578881602 (by me):
> To add a little data pointing in the direction of this bug only existing on macOS, I have synced nodes with datadirs on exFAT drives over both SATA and USB on Fedora 40 and have a long-running bitcoind node that has been running off of an exFAT SATA SSD on Fedora 40 for the past ~6 months and have never experienced any of the symptoms described in https://github.com/bitcoin/bitcoin/issues/28552 or https://github.co
...
(https://github.com/bitcoin/bitcoin/issues/31454#issuecomment-2578940904)
https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2578881602 (by me):
> To add a little data pointing in the direction of this bug only existing on macOS, I have synced nodes with datadirs on exFAT drives over both SATA and USB on Fedora 40 and have a long-running bitcoind node that has been running off of an exFAT SATA SSD on Fedora 40 for the past ~6 months and have never experienced any of the symptoms described in https://github.com/bitcoin/bitcoin/issues/28552 or https://github.co
...
📝 adlai opened a pull request: "doc: Update dependency installation for Debian/Ubuntu"
(https://github.com/bitcoin/bitcoin/pull/31621)
This is similar to the recently-pushed 8d20348 and results in slightly cleaner systems for future Debian/Ubuntu builds.
According to the description for pkg-config, "pkgconf is a replacement for pkg-config, providing additional functionality while also maintaining compatibility. This package only provides a dependency link to the pkgconf package to help with package upgrades. It can be safely removed."
Thus the relevant section of doc/build-unix.md is updated.
(https://github.com/bitcoin/bitcoin/pull/31621)
This is similar to the recently-pushed 8d20348 and results in slightly cleaner systems for future Debian/Ubuntu builds.
According to the description for pkg-config, "pkgconf is a replacement for pkg-config, providing additional functionality while also maintaining compatibility. This package only provides a dependency link to the pkgconf package to help with package upgrades. It can be safely removed."
Thus the relevant section of doc/build-unix.md is updated.
📝 achow101 opened a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622)
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatur
...
(https://github.com/bitcoin/bitcoin/pull/31622)
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatur
...
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2579034483)
I've pushed several commits to fix the sighash issues. These are also opened in their own PR #31622
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2579034483)
I've pushed several commits to fix the sighash issues. These are also opened in their own PR #31622
💬 adlai commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2579040609)
Looking over #31334 and #31335 now, it appears that updating CI for Debian should also be done; I'm not familiar with the CI infrastructure, so there's a good chance one of the regular contributors would get that done much more quickly than I.
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2579040609)
Looking over #31334 and #31335 now, it appears that updating CI for Debian should also be done; I'm not familiar with the CI infrastructure, so there's a good chance one of the regular contributors would get that done much more quickly than I.
👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2538652647)
Code review re ACK d925a9bd7a04f068ede227addbaef3025b9793b9
Great updates.
Would review again if an alternate test approach for `data/testnet4.hex` was used. I also like the idea of using early mainnet (won't need to be changed). Weighing in at ~903KB (hex), using mainnet is smaller than the 1.2MB testnet4 blocks. A nonce approach could be even nicer.
There are other areas where `.GetParams().GetConsensus()` could be replaced with `.GetConsensus()`, but I think it's prudent to prevent tou
...
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2538652647)
Code review re ACK d925a9bd7a04f068ede227addbaef3025b9793b9
Great updates.
Would review again if an alternate test approach for `data/testnet4.hex` was used. I also like the idea of using early mainnet (won't need to be changed). Weighing in at ~903KB (hex), using mainnet is smaller than the 1.2MB testnet4 blocks. A nonce approach could be even nicer.
There are other areas where `.GetParams().GetConsensus()` could be replaced with `.GetConsensus()`, but I think it's prudent to prevent tou
...
👍 luke-jr approved a pull request: "doc: Fix incorrect send RPC docs"
(https://github.com/bitcoin/bitcoin/pull/31416#pullrequestreview-2538844451)
tACK fad83e759a4
(https://github.com/bitcoin/bitcoin/pull/31416#pullrequestreview-2538844451)
tACK fad83e759a4
📝 luke-jr opened a pull request: "tracing: Rename the `MIN` macro to `_TRACEPOINT_TEST_MIN` in log_raw_p2p_msgs"
(https://github.com/bitcoin/bitcoin/pull/31623)
Inspired by: 00c1dbd26ddb816e5541c5724397015a92a3d06b (#31419)
Unless there's a reason we *don't* want the same change here...?
(https://github.com/bitcoin/bitcoin/pull/31623)
Inspired by: 00c1dbd26ddb816e5541c5724397015a92a3d06b (#31419)
Unless there's a reason we *don't* want the same change here...?
💬 hebasto commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2579545560)
Rebased due to the conflict with the merged bitcoin/bitcoin#31542.
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2579545560)
Rebased due to the conflict with the merged bitcoin/bitcoin#31542.
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908409725)
Tried compiling a `static_assert` with `mib` replaced by `-1`. Got the expected result from the sign bit:
```
/home/hodlinator/bitcoin/src/bitcoind.cpp:164:63: error: static assertion failed
164 | static_assert(std::countl_zero(static_cast<uint64_t>(-1)) >= 21);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/hodlinator/bitcoin/src/bitcoind.cpp:164:63: note: the comparison reduces to ‘(0 >= 21)’
```
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908409725)
Tried compiling a `static_assert` with `mib` replaced by `-1`. Got the expected result from the sign bit:
```
/home/hodlinator/bitcoin/src/bitcoind.cpp:164:63: error: static assertion failed
164 | static_assert(std::countl_zero(static_cast<uint64_t>(-1)) >= 21);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/hodlinator/bitcoin/src/bitcoind.cpp:164:63: note: the comparison reduces to ‘(0 >= 21)’
```
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2579621858)
Here we go!
I generated an alternate mainnet with 2016 blocks, storing timestamps and nonces. The process is explained at the top of the renamed test file: ee1dcc6074323ae361e28cf235ddfc9bf07be1da.
I did leave out some minutia for readability sake, and because things will change over time. E.g. I had to disable the `miner.isTestChain())` guard in `getblocktemplate`, I patched the CPU miner to work with taproot payout addresses https://github.com/pooler/cpuminer/pull/293 and set the coinba
...
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2579621858)
Here we go!
I generated an alternate mainnet with 2016 blocks, storing timestamps and nonces. The process is explained at the top of the renamed test file: ee1dcc6074323ae361e28cf235ddfc9bf07be1da.
I did leave out some minutia for readability sake, and because things will change over time. E.g. I had to disable the `miner.isTestChain())` guard in `getblocktemplate`, I patched the CPU miner to work with taproot payout addresses https://github.com/pooler/cpuminer/pull/293 and set the coinba
...
👍 dergoegge approved a pull request: "Safegcd-based modular inverses in MuHash3072"
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2539393251)
tACK ad67fd2e0bfa6f43f350066596b6cca146391362
Only minor changes since my [last review](https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2104888365).
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2539393251)
tACK ad67fd2e0bfa6f43f350066596b6cca146391362
Only minor changes since my [last review](https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2104888365).
💬 l0rinc commented on pull request "init,log: Unify block index log line":
(https://github.com/bitcoin/bitcoin/pull/31616#issuecomment-2579651695)
@maflcko do you mean removing these logs in the first place since they're redundant?
(https://github.com/bitcoin/bitcoin/pull/31616#issuecomment-2579651695)
@maflcko do you mean removing these logs in the first place since they're redundant?