Bitcoin Core Github
44 subscribers
120K links
Download Telegram
πŸ“ maflcko opened a pull request: "log: [refactor] Use info level for init logs"
(https://github.com/bitcoin/bitcoin/pull/32967)
Many of the normal, and expected init logs, which are run once after startup use the deprecated alias of `LogInfo`.

Fix that by using `LogInfo` directly, which is a refactor, except for a few log lines that also have `__func__` removed.

(Also remove the unused trailing `\n` char while touching those logs)
πŸ’¬ stratospher commented on issue "Intermittent failure in rpc_invalidateblock.py assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7) [ AssertionError: not(24 == 7)]":
(https://github.com/bitcoin/bitcoin/issues/32965#issuecomment-3068592237)
> 2025-07-12T13:43:21.8573662Z node1 2025-07-12T13:43:20.090914Z [msghand] [D:\a\bitcoin\bitcoin\src\net.cpp:3870] [void __cdecl CConnman::PushMessage(class CNode *,struct CSerializedNetMsg &&)] [net] sending block (260 bytes) peer=0
2025-07-12T13:43:21.8574419Z node0 2025-07-12T13:43:20.091379Z [msghand] [D:\a\bitcoin\bitcoin\src\net_processing.cpp:5939] [bool __cdecl `anonymous-namespace'::PeerManagerImpl::SendMessages(class CNode *)] [net] Requesting block 72876656a1468aa0784c5624a377651fb
...
πŸ’¬ fanquake commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2204337893)
LogWarning?
πŸ’¬ fanquake commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2204338168)
LogWarning?
πŸ’¬ maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2204346812)
thx, reverted. (I'll do the warn level in a follow-up, so that this is limited to just the info level)
πŸ“ stratospher opened a pull request: "test: fix intermittent failure in rpc_invalidateblock.py"
(https://github.com/bitcoin/bitcoin/pull/32968)
resolves #32965.

node1 (with 24 blocks) causes node0 (with 6 blocks + 1 extra header) to silently reorg. so disconnect node0 and node1 to avoid silent reorgs.
πŸ’¬ purpleKarrot commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3068613759)
NACK

After working with the API for a few days and reviewing the various language bindings listed in the PR summary, I found that the API requires some fundamental changes in order to reduce the amount of glue code required in language bindings and client code. I wrote a rather detailed analysis here: https://njump.me/naddr1qvzqqqr4gupzqrcxrljwdpfz2qn5a57hse6ez6pkd34pe0wpeskmktt2p62yeketqqvxy6t5vdhkjmntv4exuetv94shq6fdwfjhv6t9wuxrjull
πŸ’¬ maflcko commented on pull request "test: fix intermittent failure in rpc_invalidateblock.py":
(https://github.com/bitcoin/bitcoin/pull/32968#discussion_r2204408981)
This means the remainder of the test, the state of the two nodes shouldn't affect each other. So the reconsiderblock+test

```
assert_equal(self.nodes[0].getbestblockhash(), blockhash_6)
assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 6)
assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7)
```

Can be moved after this line to disconnect the nodes? This would bundle the tests more nicely, but no strong opinion.
πŸ’¬ stratospher commented on issue "Intermittent failure in rpc_invalidateblock.py assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7) [ AssertionError: not(24 == 7)]":
(https://github.com/bitcoin/bitcoin/issues/32965#issuecomment-3068706510)
# Reconsider the header
self.nodes[0].reconsiderblock(block.hash)
import time; time.sleep(200)
# Since header doesn't have block data, it can't be chain tip
# Check if it's possible for an ancestor (with block data) to be the chain tip
assert_equal(self.nodes[0].getbestblockhash(), blockhash_6)
assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 6)
assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7)

I am not sure wh
...
πŸ’¬ maflcko commented on pull request "test: fix intermittent failure in rpc_invalidateblock.py":
(https://github.com/bitcoin/bitcoin/pull/32968#issuecomment-3068710732)
lgtm ACK 4aa0e4d9ce6283665b1aac185da67543b8df606e

Also, left a nit/questsion
πŸ’¬ maflcko commented on issue "Intermittent failure in rpc_invalidateblock.py assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7) [ AssertionError: not(24 == 7)]":
(https://github.com/bitcoin/bitcoin/issues/32965#issuecomment-3068720253)
You can test this by replacing the newly added `disconnect_nodes` in your fix with `self.nodes[0].reconsiderblock(block.hash)`.
πŸ‘ fanquake approved a pull request: "cmake: Drop no longer necessary "cmakeMinimumRequired" object"
(https://github.com/bitcoin/bitcoin/pull/32954#pullrequestreview-3015664087)
ACK 12a6959892cb24b940b3579828f2066651572153 - has been unneeded since it was introduced (minimum was already 3.22).
πŸš€ fanquake merged a pull request: "cmake: Drop no longer necessary "cmakeMinimumRequired" object"
(https://github.com/bitcoin/bitcoin/pull/32954)
πŸ’¬ TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3068782737)
Thank you for the review @stringintech!

Rebased 690a5dac223ed18a65c9d9e6c535466cc3ad4511 -> 52bab146a5045899ea6800305fa6d9b4efdcc6bd ([kernelApi_42](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_42) -> [kernelApi_43](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_43), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_42..kernelApi_43))

Updated 52bab146a5045899ea6800305fa6d9b4efdcc6bd -> 267a7b3f321304f75e8c47e380da49ba9c64bc84 ([kernelApi_43](https://gith
...
πŸ’¬ maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#issuecomment-3068819299)
re-ACK 11546183c70def6c0aa539642fd1c9ada3d46840 πŸ—»

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 11546183c70def6c0aa5
...
❀1
πŸ’¬ maflcko commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2204495759)

exists in disk -> exists on disk [β€œin disk” is nonstandard; use β€œon disk”]


An alternative wording could be "if ... was persisted, clear it"
πŸ’¬ hebasto commented on pull request "ci: Avoid cd into build dir":
(https://github.com/bitcoin/bitcoin/pull/32880#issuecomment-3068845648)
> > ACK [fa0eca8](https://github.com/bitcoin/bitcoin/commit/fa0eca82ec1222ec1c68835ce7acdf9c8c4740ad), I have reviewed the code and it looks OK.
>
> I think you reviewed a commit that still had the pre-existing $GOAL bug?

I did review the correct branch but accidentally copied the wrong hash from my Sublime Merge instance. Now fixed.
πŸ’¬ TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3068922992)
Re https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3068613759

Thanks for writing all of that up and your detailed tour @purpleKarrot. I think you raise some excellent points on your blog, but I am not sure how I am to interpret your NACK here.

You mention that fundamental changes are required, but after reading some of your proposed changes in `/btck` I am not sure how materially different those are from what is proposed here. I think naming conventions is probably the easiest w
...
πŸš€ fanquake merged a pull request: "ci: Avoid cd into build dir"
(https://github.com/bitcoin/bitcoin/pull/32880)
πŸ’¬ purpleKarrot commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3069008389)
> am not sure how I am to interpret your NACK here.

Just as "I think this PR should not be merged in its current form." I definitely do agree with the approach of adding a C API.

Regarding the other points, maybe we should have a private discussion.