Bitcoin Core Github
42 subscribers
125K links
Download Telegram
⚠️ plebhash opened an issue: "Mining IPC `createNewBlock` should not return before IBD is over"
(https://github.com/bitcoin/bitcoin/issues/33994)
### Please describe the feature you'd like to see added.

Mining IPC `createNewBlock` should not return before IBD is over

### Is your feature related to a problem, if so please describe it.

on SRI [`bitcoin_core_sv2`](https://github.com/stratum-mining/sv2-apps/tree/main/bitcoin-core-sv2) crate, we want to wait for IBD is over as part of the bootstrapping processes

discussing with @ismaelsadeeq and @Shourya742 during BTrust dev day, we came to the conclusion that `createNewBlock` shouldn't ev
...
💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2580917867)
Thanks! Reworked.
🤔 alexanderwiederin reviewed a pull request: "kernel: Expose reusable `PrecomputedTransactionData` in script validation"
(https://github.com/bitcoin/bitcoin/pull/33891#pullrequestreview-3529821349)
ACK https://github.com/bitcoin/bitcoin/pull/33891/commits/17d3575cfcb41adfd364e7a1912a4701786f7455
💬 alexanderwiederin commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2580952377)
nit: wonder if "efficient" here could be misleading.
💬 fanquake commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3601824876)
Note that 0972f5504021b482b27523fd3bcb8036cf6b439c has broken our `gen-manpages.py` script, by appending additional text (for some bins) to the line containing the version info.
💬 stickies-v commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2581001217)
> I think there will always be a possibility to argue one way or the other,

I agree, so even though I have a different view, it's not that practically important so this shouldn't be a blocker. This PR is an improvement with either approach.

> it seems like it should propagate the error up as a fatal one.

I don't think it needs to be propagated as fatal, the failing subsystem / callsite can just log its issue as a warning (when it has no concept or knowledge on fatality), and then higher
...
💬 stickies-v commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#issuecomment-3601835371)
ACK fa45a1503eee603059166071857215ea9bd7242a
👍 rkrux approved a pull request: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3530005825)
lgtm re-ACK 76c092ff805833a9adf84f669f0455bc2e0bba8b
💬 brunoerg commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2581145624)
Just ran a mutation testing for this PR and this is the only unkilled mutant - feel free to ignore if it doesn't make sense to address:

```diff
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index f734296b24..13ddbb672f 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -880,7 +880,7 @@ public:
for (const CTransactionRef& tx : m_block_template->block.vtx | std::views::drop(1)) {
auto ref_count{m_node.template_tx_refs.find(tx)};

...
💬 m3dwards commented on pull request "doc: Add `x86_64-w64-mingw32ucrt` triplet to `depends/README.md`":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3602078949)
ACK ec8eb013a9bfceb324b309f13b8946b05292a993
🚀 fanquake merged a pull request: "log: Use more severe log level (warn/err) where appropriate"
(https://github.com/bitcoin/bitcoin/pull/33960)
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3602119940)
rebased and force-pushed to resolve the CI test failures.

Also addressed feedback from theStack

- moved the `validate_snapshot_import` and `complete_background_validation` methods into an own refactoring commit
- added a comma after `["-fastprune", "-prune=1"]` for smaller future diff if another node is added
- deduplicated the expected error message within `test_backup_during_background_sync_pruned_node` so it doesn't have to be adapted twice if it ever changes
💬 ismaelsadeeq commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602165226)
@plebhash It will be nice to get to the root of the issue and see whether it's bitcoin core or from the template provider.
Can you please provide the logs and setups accessible in a text file and not a discord link I think some people might not have discord.
cc @Sjors I attempt to reproduce the failure but was not successful yet.
blocking `createnewblock` from creating template during ibd is potential solution and would also delegate the responsibility of polling isinitialblockdownload to the
...
💬 sdaftuar commented on pull request "fuzz: gate mempool entry based on weight":
(https://github.com/bitcoin/bitcoin/pull/33985#issuecomment-3602179098)
ACK 804329400a73df00dfd7a5209c659d4a22b9ce47
💬 fanquake commented on pull request "fuzz: gate mempool entry based on weight":
(https://github.com/bitcoin/bitcoin/pull/33985#issuecomment-3602185685)
cc @dergoegge
💬 Sjors commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3602275695)
Rebased on the latest #33965 which allowed for some simplifications:
- `ClampOptions` no longer takes a `MiningArgs` argument
- no modifications to RPC code
- the new `block_max_weight` option on `BlockCreateOptions` will no longer be ignored if a caller sets it, only the tx_pool fuzzer does.

We could trivially make the new `block_max_weight` option available to IPC clients, but I don't think we should encourage its use.

Note that I brought back `ApplyArgsManOptions` in `miner.cpp`, but
...
💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3602323654)
Rebased after #33591 because #33966 has a trivial merge conflict with it and I want to keep that cleanly based on this PR.
💬 ajtowns commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3602342513)
> P2SH allows non-solvable redeem scripts with low sigop counts (PR https://github.com/bitcoin/bitcoin/pull/4365), while legacy scripts still require solvability. This inconsistency is hard to justify, especially since both rely on sigop limits for security.

> I agree. I think my next step would be to PR a change to `AreInputsStandard` to bring P2SH and legacy script policy into alignment.

I think I disagree with this -- we allow spends of fairly arbitrary p2sh scripts, but limit legacy spends
...
💬 Sjors commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602376905)
The problem is that `ChainstateManager::IsInitialBlockDownload()` uses a fairly relaxed heuristic:

https://github.com/bitcoin/bitcoin/blob/d0f6d9953a15d7c7111d46dcb76ab2bb18e5dee3/src/validation.cpp#L1998-L2023

We could make this function much more precise by having it consider the most proof-of-work (not invalid) header chain.

But that might have impacts in other places in the codebase, potentially causing things to get stuck longer than needed just because we're missing a few recent blocks.
...
💬 sipa commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602401780)
You can't use "active_tip == best_known_block_header" as condition for mining, because an attacker could mine a block, relay the header, and never relay the block. Sure, very expensive, but if the outcome is shutting down other miners, some joker might try it.

Bitcoin nodes are *always* synchronizing, and unless you have very strong evidence to the contrary, you should act as if your active tip is the best block on the chain. If we had evidence of a better block, it would be our best tip instea
...