👋 instagibbs's pull request is ready for review: "policy: don't CheckEphemeralSpends on reorg"
(https://github.com/bitcoin/bitcoin/pull/33616)
(https://github.com/bitcoin/bitcoin/pull/33616)
💬 instagibbs commented on pull request "policy: don't CheckEphemeralSpends on reorg":
(https://github.com/bitcoin/bitcoin/pull/33616#issuecomment-3597968747)
Added some more coverage and now has no dependencies.
(https://github.com/bitcoin/bitcoin/pull/33616#issuecomment-3597968747)
Added some more coverage and now has no dependencies.
💬 hebasto commented on pull request "depends, doc: Add `tcl` as build dependency for `sqlite` package":
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3597986268)
@janb84
> This doesn't work for me on ubuntu 24.04, if I only install what is specified and then execute:
>
> ```shell
> gmake -C depends sqlite CC=gcc-14 CXX=g++-14
> ```
The example above assumes that the [`gcc-14`](https://packages.ubuntu.com/noble/gcc-14) and [`g++-14`](https://packages.ubuntu.com/noble/g++-14) packages are installed.
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3597986268)
@janb84
> This doesn't work for me on ubuntu 24.04, if I only install what is specified and then execute:
>
> ```shell
> gmake -C depends sqlite CC=gcc-14 CXX=g++-14
> ```
The example above assumes that the [`gcc-14`](https://packages.ubuntu.com/noble/gcc-14) and [`g++-14`](https://packages.ubuntu.com/noble/g++-14) packages are installed.
💬 janb84 commented on pull request "depends, doc: Add `tcl` as build dependency for `sqlite` package":
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3598060500)
> @janb84
>
> > This doesn't work for me on ubuntu 24.04, if I only install what is specified and then execute:
> > ```shell
> > gmake -C depends sqlite CC=gcc-14 CXX=g++-14
> > ```
>
> The example above assumes that the [`gcc-14`](https://packages.ubuntu.com/noble/gcc-14) and [`g++-14`](https://packages.ubuntu.com/noble/g++-14) packages are installed.
Ah Sorry, my mistake, took your example to literally.
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3598060500)
> @janb84
>
> > This doesn't work for me on ubuntu 24.04, if I only install what is specified and then execute:
> > ```shell
> > gmake -C depends sqlite CC=gcc-14 CXX=g++-14
> > ```
>
> The example above assumes that the [`gcc-14`](https://packages.ubuntu.com/noble/gcc-14) and [`g++-14`](https://packages.ubuntu.com/noble/g++-14) packages are installed.
Ah Sorry, my mistake, took your example to literally.
🤔 janb84 reviewed a pull request: "depends, doc: Add `tcl` as build dependency for `sqlite` package"
(https://github.com/bitcoin/bitcoin/pull/33975#pullrequestreview-3526330943)
ACK 632cf872a584e4a373a669aca5b885f63a2b7add
PR adds `tcl` as packages to install to compile depends.
Have confirmed change on ubuntu 24.04
(https://github.com/bitcoin/bitcoin/pull/33975#pullrequestreview-3526330943)
ACK 632cf872a584e4a373a669aca5b885f63a2b7add
PR adds `tcl` as packages to install to compile depends.
Have confirmed change on ubuntu 24.04
💬 sdaftuar commented on pull request "fuzz: gate mempool entry based on weight":
(https://github.com/bitcoin/bitcoin/pull/33985#issuecomment-3598198410)
Looks right to me, and I was able to generate a fuzz crash on master that was fixed by this PR. Going to continue fuzzing for a bit to see if there are any other issues.
(https://github.com/bitcoin/bitcoin/pull/33985#issuecomment-3598198410)
Looks right to me, and I was able to generate a fuzz crash on master that was fixed by this PR. Going to continue fuzzing for a bit to see if there are any other issues.
⚠️ manuelbarraza2202-debug opened an issue: "Top Bitcoin recovery expert helping victims restore stolen digital funds. Visit TECHY FORCE CYBER RETRIEVAL"
(https://github.com/bitcoin/bitcoin/issues/33988)
### Motivation
The world of cryptocurrency has been marred by numerous scams, with unsuspecting investors losing substantial amounts to fraudulent schemes. One such scam involved a bogus job opportunity that demanded a hefty $49,000 "training fee" in USDC (USD Coin). Many victims of this scam had given up hope of ever recovering their lost investment. However, a recent success story has demonstrated that, with the right approach, it is possible to recover significant amounts of stolen funds. Th
...
(https://github.com/bitcoin/bitcoin/issues/33988)
### Motivation
The world of cryptocurrency has been marred by numerous scams, with unsuspecting investors losing substantial amounts to fraudulent schemes. One such scam involved a bogus job opportunity that demanded a hefty $49,000 "training fee" in USDC (USD Coin). Many victims of this scam had given up hope of ever recovering their lost investment. However, a recent success story has demonstrated that, with the right approach, it is possible to recover significant amounts of stolen funds. Th
...
🤔 mzumsande reviewed a pull request: "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us"
(https://github.com/bitcoin/bitcoin/pull/33723#pullrequestreview-3526719032)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
agree with laanwj, not enough trust.
(for the record I still don't view the filtering out newest versions as a major issue, slightly different policies between the different DNS seeds could even be beneficial in some situations)
(https://github.com/bitcoin/bitcoin/pull/33723#pullrequestreview-3526719032)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
agree with laanwj, not enough trust.
(for the record I still don't view the filtering out newest versions as a major issue, slightly different policies between the different DNS seeds could even be beneficial in some situations)
💬 mzumsande commented on pull request "p2p: avoid traversing blocks (twice) during IBD":
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3598646376)
> At the least the tx orphanage should not require work when it is empty
That part was merged with #32827
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3598646376)
> At the least the tx orphanage should not require work when it is empty
That part was merged with #32827
💬 laanwj commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3598664888)
> So that seems like [compute_builtin_object_size](https://github.com/gcc-mirror/gcc/blob/c9cd41fba9ebd288c4f101e4b99da934bcb96a11/gcc/tree-object-size.cc#L1126) is returning a different value based on the host arch?
Wonder if there's any windows-specific code paths for that in mingw64? After all, we're not seeing it on the other architectures.
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3598664888)
> So that seems like [compute_builtin_object_size](https://github.com/gcc-mirror/gcc/blob/c9cd41fba9ebd288c4f101e4b99da934bcb96a11/gcc/tree-object-size.cc#L1126) is returning a different value based on the host arch?
Wonder if there's any windows-specific code paths for that in mingw64? After all, we're not seeing it on the other architectures.
💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3598680323)
Concept ACK. Would suggest updating `-blockreservedweight` documentation to say it only affects RPC mining clients, not IPC clients.
I think implementation of this could be improved a bit, depending on if you think it would be useful for IPC clients to "have a way to signal their intent to use the node default"?
If yes, would suggest just making the field `std::optional`:
<details><summary>diff</summary>
<p>
```diff
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -78,11 +78,
...
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3598680323)
Concept ACK. Would suggest updating `-blockreservedweight` documentation to say it only affects RPC mining clients, not IPC clients.
I think implementation of this could be improved a bit, depending on if you think it would be useful for IPC clients to "have a way to signal their intent to use the node default"?
If yes, would suggest just making the field `std::optional`:
<details><summary>diff</summary>
<p>
```diff
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -78,11 +78,
...
👍 hodlinator approved a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3526806887)
ACK 8af0a3a72c8af781bebed7289657c637220df893
While the benefit of this change to consensus code is low, it's been simplified back to a more easily reviewable version.
The benchmark now has two parts, one that detects mutation issues and one which does not, corresponding to the non-witness & witness merkle root computations performed by Bitcoin Core on mainnet.
### One extra level of paranoia?
I think this change really is simple enough to approve as-is. It's maybe not the same risk-level as
...
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3526806887)
ACK 8af0a3a72c8af781bebed7289657c637220df893
While the benefit of this change to consensus code is low, it's been simplified back to a more easily reviewable version.
The benchmark now has two parts, one that detects mutation issues and one which does not, corresponding to the non-witness & witness merkle root computations performed by Bitcoin Core on mainnet.
### One extra level of paranoia?
I think this change really is simple enough to approve as-is. It's maybe not the same risk-level as
...
💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2578500589)
Looks good!
nit: With your version of having 2 benchmarks within the same function, `-filter='MerkleRoot.*'` in the commit message can be simplified to `-filter='MerkleRoot'`.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2578500589)
Looks good!
nit: With your version of having 2 benchmarks within the same function, `-filter='MerkleRoot.*'` in the commit message can be simplified to `-filter='MerkleRoot'`.
💬 ryanofsky commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3598781679)
Concept ACK 517a9b23283fee0a861578b2686a71c48a4b67b4, but I had some questions on #33965 which this builds on, and this PR will be affected by what happens to that.
Overall most changes here seem very good: it's nice to introduce a MiningArgs struct and move handling of mining options out of `init.cpp`. Also nice to port more code to use `interfaces::Mining`. Other changes seem less positive. Before, there was a clear separation of which options were controllable by `interfaces/mining.h` and
...
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3598781679)
Concept ACK 517a9b23283fee0a861578b2686a71c48a4b67b4, but I had some questions on #33965 which this builds on, and this PR will be affected by what happens to that.
Overall most changes here seem very good: it's nice to introduce a MiningArgs struct and move handling of mining options out of `init.cpp`. Also nice to port more code to use `interfaces::Mining`. Other changes seem less positive. Before, there was a clear separation of which options were controllable by `interfaces/mining.h` and
...
📝 AfuroIsCrazy opened a pull request: "Update copyright"
(https://github.com/bitcoin/bitcoin/pull/33989)
(https://github.com/bitcoin/bitcoin/pull/33989)
📝 theStack opened a pull request: "test: check that peer's announced starting height is remembered"
(https://github.com/bitcoin/bitcoin/pull/33990)
Note that the announced starting height of a peer is neither verified nor used in any other logic, so reporting a bogus value [1] as done in the test doesn't have any consequences -- it's only used for inspection via the `getpeerinfo` RPC and in some debug messages (see also e.g. https://github.com/bitcoin-dot-org/Bitcoin.org/issues/1387#issuecomment-252934859). Admittedly this is not terribly important, but I'd still think having test coverage for this simple reporting functionality is better t
...
(https://github.com/bitcoin/bitcoin/pull/33990)
Note that the announced starting height of a peer is neither verified nor used in any other logic, so reporting a bogus value [1] as done in the test doesn't have any consequences -- it's only used for inspection via the `getpeerinfo` RPC and in some debug messages (see also e.g. https://github.com/bitcoin-dot-org/Bitcoin.org/issues/1387#issuecomment-252934859). Admittedly this is not terribly important, but I'd still think having test coverage for this simple reporting functionality is better t
...
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3599647251)
> Shouldn't this be removing the other CI (#33764):
>
> > MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
Removed.
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3599647251)
> Shouldn't this be removing the other CI (#33764):
>
> > MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
Removed.
📝 codomposer opened a pull request: "test: Add comprehensive txindex reorg test coverage"
(https://github.com/bitcoin/bitcoin/pull/33991)
# Add comprehensive txindex reorg test coverage
## Motivation
This PR adds a new functional test `feature_txindex_reorg.py` to improve test coverage for the transaction index (`-txindex`) behavior during chain reorganizations. Currently, while txindex functionality is tested in various scenarios, there is limited coverage specifically focused on its behavior during deep reorgs, concurrent access patterns, and edge cases that could affect reliability.
The transaction index is a critical
...
(https://github.com/bitcoin/bitcoin/pull/33991)
# Add comprehensive txindex reorg test coverage
## Motivation
This PR adds a new functional test `feature_txindex_reorg.py` to improve test coverage for the transaction index (`-txindex`) behavior during chain reorganizations. Currently, while txindex functionality is tested in various scenarios, there is limited coverage specifically focused on its behavior during deep reorgs, concurrent access patterns, and edge cases that could affect reliability.
The transaction index is a critical
...
✅ pinheadmz closed a pull request: "test: Add comprehensive txindex reorg test coverage"
(https://github.com/bitcoin/bitcoin/pull/33991)
(https://github.com/bitcoin/bitcoin/pull/33991)
💬 pinheadmz commented on pull request "test: Add comprehensive txindex reorg test coverage":
(https://github.com/bitcoin/bitcoin/pull/33991#issuecomment-3599888466)
Pretty sure this is just AI slop. "Gittensor" is kinda dead give away.
(https://github.com/bitcoin/bitcoin/pull/33991#issuecomment-3599888466)
Pretty sure this is just AI slop. "Gittensor" is kinda dead give away.