📝 fanquake opened a pull request: "[28.x] Backport & finalise 28.3"
(https://github.com/bitcoin/bitcoin/pull/33613)
Backports:
* #33581
Plus the changes to finalise `v28.3`
(https://github.com/bitcoin/bitcoin/pull/33613)
Backports:
* #33581
Plus the changes to finalise `v28.3`
💬 fanquake commented on pull request "ci: Properly include $FILE_ENV in DEPENDS_HASH":
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3398132819)
Backported to `28.x` in #33613.
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3398132819)
Backported to `28.x` in #33613.
💬 m3dwards commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3398197163)
@maflcko hopefully this should sort it: https://github.com/m3dwards/bitcoin-core-with-ci/commit/5131564636ae971fb1ab68aeb980824af432fdf4
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3398197163)
@maflcko hopefully this should sort it: https://github.com/m3dwards/bitcoin-core-with-ci/commit/5131564636ae971fb1ab68aeb980824af432fdf4
💬 Sjors commented on pull request "miner: empty mempool special case for waitNext()":
(https://github.com/bitcoin/bitcoin/pull/33566#issuecomment-3398246786)
Great idea, I integrated @Raimo33's approach.
(https://github.com/bitcoin/bitcoin/pull/33566#issuecomment-3398246786)
Great idea, I integrated @Raimo33's approach.
💬 Raimo33 commented on pull request "miner: fix empty mempool case for waitNext()":
(https://github.com/bitcoin/bitcoin/pull/33566#issuecomment-3398250482)
> Great idea, I integrated @Raimo33's approach.
please add me as co-author or cherry-pick my commit
(https://github.com/bitcoin/bitcoin/pull/33566#issuecomment-3398250482)
> Great idea, I integrated @Raimo33's approach.
please add me as co-author or cherry-pick my commit
💬 hebasto commented on pull request "Bugfix: Only use git for build info if the repository is actually the right one":
(https://github.com/bitcoin/bitcoin/pull/32217#discussion_r2426835503)
This comment is still unaddressed.
(https://github.com/bitcoin/bitcoin/pull/32217#discussion_r2426835503)
This comment is still unaddressed.
💬 hebasto commented on pull request "Bugfix: Only use git for build info if the repository is actually the right one":
(https://github.com/bitcoin/bitcoin/pull/32217#issuecomment-3398257517)
> Also seems like the obvious time to document `BITCOIN_GENBUILD_NO_GIT` (#31999)
This is still unaddressed.
(https://github.com/bitcoin/bitcoin/pull/32217#issuecomment-3398257517)
> Also seems like the obvious time to document `BITCOIN_GENBUILD_NO_GIT` (#31999)
This is still unaddressed.
💬 Sjors commented on pull request "miner: fix empty mempool case for waitNext()":
(https://github.com/bitcoin/bitcoin/pull/33566#issuecomment-3398259043)
@Raimo33 I did, see `Co-authored-by` in commit message.
(https://github.com/bitcoin/bitcoin/pull/33566#issuecomment-3398259043)
@Raimo33 I did, see `Co-authored-by` in commit message.
📝 waketraindev opened a pull request: "rpc, test: fix "internal" label check in importdescriptors and extend test"
(https://github.com/bitcoin/bitcoin/pull/33614)
Fixes a logic issue in `ProcessDescriptorImport()` where descriptors with `"internal": false` and a label were incorrectly rejected.
The check now uses `internal.value_or(false)` so labels are only disallowed when `internal` is `true`.
Tests updated to cover both cases
(https://github.com/bitcoin/bitcoin/pull/33614)
Fixes a logic issue in `ProcessDescriptorImport()` where descriptors with `"internal": false` and a label were incorrectly rejected.
The check now uses `internal.value_or(false)` so labels are only disallowed when `internal` is `true`.
Tests updated to cover both cases
👋 waketraindev's pull request is ready for review: "rpc, test: fix "internal" label check in importdescriptors and extend test"
(https://github.com/bitcoin/bitcoin/pull/33614)
(https://github.com/bitcoin/bitcoin/pull/33614)
⚠️ Crypt-iQ opened an issue: "ASAN use-after-free in `m_reconnections`"
(https://github.com/bitcoin/bitcoin/issues/33615)
I ran into this while shutting my node down during IBD. I think it's benign.
What happens:
- `CConnman` is started [here](https://github.com/bitcoin/bitcoin/blob/64a7c7cbb975cb3c3f25a3f784779f32cd95ebaa/src/init.cpp#L2183).
- This initializes `semOutbound` [here](https://github.com/bitcoin/bitcoin/blob/64a7c7cbb975cb3c3f25a3f784779f32cd95ebaa/src/net.cpp#L3361).
- While `CConnman` is active, `m_reconnections` is added to in `DisconnectNodes` [here](https://github.com/bitcoin/bitcoin/blob/64a7
...
(https://github.com/bitcoin/bitcoin/issues/33615)
I ran into this while shutting my node down during IBD. I think it's benign.
What happens:
- `CConnman` is started [here](https://github.com/bitcoin/bitcoin/blob/64a7c7cbb975cb3c3f25a3f784779f32cd95ebaa/src/init.cpp#L2183).
- This initializes `semOutbound` [here](https://github.com/bitcoin/bitcoin/blob/64a7c7cbb975cb3c3f25a3f784779f32cd95ebaa/src/net.cpp#L3361).
- While `CConnman` is active, `m_reconnections` is added to in `DisconnectNodes` [here](https://github.com/bitcoin/bitcoin/blob/64a7
...
📝 instagibbs opened a pull request: "2025 10 bypass checkephemeral"
(https://github.com/bitcoin/bitcoin/pull/33616)
Similar reasoning to https://github.com/bitcoin/bitcoin/pull/33504
During a deeper reorg it's possible that a long sequence of dust-having transactions that are connected in a linear fashion, and currently each subsequent "generation" will be rejected on reorg, even though the dust would be spent usually by another child transaction. This could evict a large amount of competitive fees via normal means.
PreCheck based `PreCheckEphemeralSpends` is left in place because we wouldn't have relay
...
(https://github.com/bitcoin/bitcoin/pull/33616)
Similar reasoning to https://github.com/bitcoin/bitcoin/pull/33504
During a deeper reorg it's possible that a long sequence of dust-having transactions that are connected in a linear fashion, and currently each subsequent "generation" will be rejected on reorg, even though the dust would be spent usually by another child transaction. This could evict a large amount of competitive fees via normal means.
PreCheck based `PreCheckEphemeralSpends` is left in place because we wouldn't have relay
...
💬 fjahr commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-3398490065)
Turning it off and on again finally worked :p
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-3398490065)
Turning it off and on again finally worked :p
💬 janb84 commented on pull request "build: Bump clang minimum supported version to 17":
(https://github.com/bitcoin/bitcoin/pull/33555#issuecomment-3398591114)
ConceptACK fa0fa0f70087d08fe5a54832b96799bd14293279
PR Sets the clang minimum supported version to 17 and drop some work arounds for clang 16. Given that supported OS-es have already a minimal clang version of 17, this should not be a problem.
Would like to see confirmation that the fuzz test works as expected, by a fuzz specialist.
- code review ✅
- GUIX builds ✅
- normal build ✅
- ran tests ✅
Ps. NixOS 25.05 has Clang version 19 as default.
<details><summary> Guix Build O
...
(https://github.com/bitcoin/bitcoin/pull/33555#issuecomment-3398591114)
ConceptACK fa0fa0f70087d08fe5a54832b96799bd14293279
PR Sets the clang minimum supported version to 17 and drop some work arounds for clang 16. Given that supported OS-es have already a minimal clang version of 17, this should not be a problem.
Would like to see confirmation that the fuzz test works as expected, by a fuzz specialist.
- code review ✅
- GUIX builds ✅
- normal build ✅
- ran tests ✅
Ps. NixOS 25.05 has Clang version 19 as default.
<details><summary> Guix Build O
...
👍 stickies-v approved a pull request: "refactor: Construct g_verify_flag_names on first use"
(https://github.com/bitcoin/bitcoin/pull/33600#pullrequestreview-3332643263)
ACK faa9d10c84bc6b465cbca266468990cc716b4300
I would prefer the `constexpr` approach, but either approach is acceptable to me.
(https://github.com/bitcoin/bitcoin/pull/33600#pullrequestreview-3332643263)
ACK faa9d10c84bc6b465cbca266468990cc716b4300
I would prefer the `constexpr` approach, but either approach is acceptable to me.
💬 stickies-v commented on pull request "refactor: Construct g_verify_flag_names on first use":
(https://github.com/bitcoin/bitcoin/pull/33600#discussion_r2427048878)
An alternative approach would be to use a `constexpr` array and make the whole thing compile-time? As a nice benefit, removes the `FLAG_NAME` macro which imo doesn't simplify things enough to be worth it.
<details>
<summary>git diff on faa9d10c84</summary>
```diff
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index abd99fc365..731c0a070f 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -2163,36 +2163,6 @@ size_t CountWitnessSigOps(con
...
(https://github.com/bitcoin/bitcoin/pull/33600#discussion_r2427048878)
An alternative approach would be to use a `constexpr` array and make the whole thing compile-time? As a nice benefit, removes the `FLAG_NAME` macro which imo doesn't simplify things enough to be worth it.
<details>
<summary>git diff on faa9d10c84</summary>
```diff
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index abd99fc365..731c0a070f 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -2163,36 +2163,6 @@ size_t CountWitnessSigOps(con
...
💬 janb84 commented on pull request "rpc, test: fix "internal" label check in importdescriptors and extend test":
(https://github.com/bitcoin/bitcoin/pull/33614#discussion_r2427075461)
NIT: would rather see `internal == true` imho better readability.
```suggestion
if (internal == true && data.exists("label")) {
```
(https://github.com/bitcoin/bitcoin/pull/33614#discussion_r2427075461)
NIT: would rather see `internal == true` imho better readability.
```suggestion
if (internal == true && data.exists("label")) {
```
💬 janb84 commented on pull request "rpc, test: fix "internal" label check in importdescriptors and extend test":
(https://github.com/bitcoin/bitcoin/pull/33614#discussion_r2427079894)
NIT: restore this and use the pattern that is used for the rest of the tests. Change the `import_request` slightly e.g:
``` C
self.log.info("Test can import a p2pkh descriptor with internal explicit set to false ")
self.test_importdesc({**import_request, "internal": False}, success=True)
```
(https://github.com/bitcoin/bitcoin/pull/33614#discussion_r2427079894)
NIT: restore this and use the pattern that is used for the rest of the tests. Change the `import_request` slightly e.g:
``` C
self.log.info("Test can import a p2pkh descriptor with internal explicit set to false ")
self.test_importdesc({**import_request, "internal": False}, success=True)
```
💬 waketraindev commented on pull request "rpc, test: fix "internal" label check in importdescriptors and extend test":
(https://github.com/bitcoin/bitcoin/pull/33614#issuecomment-3398759053)
Resolved nits, internal checks for true, test updated.
(https://github.com/bitcoin/bitcoin/pull/33614#issuecomment-3398759053)
Resolved nits, internal checks for true, test updated.
💬 sipa commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3399048111)
Imagine the following state:
```mermaid
graph BT
g["genesis"];
f["fork_point"];
a["ActiveTip()"];
b["pindexBestKnownBlock"];
l["pindexLastCommonBlock"];
p["P"];
f-->g;
a-->f;
l-->p;
b-->p;
p-->f;
```
In this situation, I think we want to update `pindexLastCommonBlock` to be `P`, but the current code will select `fork_point`. Is this a situation that can occur?
I think that the logic we actually want is selecting the more-work among `LastCommonAncestor(pindexBestKnownBlock
...
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3399048111)
Imagine the following state:
```mermaid
graph BT
g["genesis"];
f["fork_point"];
a["ActiveTip()"];
b["pindexBestKnownBlock"];
l["pindexLastCommonBlock"];
p["P"];
f-->g;
a-->f;
l-->p;
b-->p;
p-->f;
```
In this situation, I think we want to update `pindexLastCommonBlock` to be `P`, but the current code will select `fork_point`. Is this a situation that can occur?
I think that the logic we actually want is selecting the more-work among `LastCommonAncestor(pindexBestKnownBlock
...