💬 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
...

💬 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.

💬 maflcko commented on pull request "[POC] ci: Skip compilation when running static code analysis":
(https://github.com/bitcoin/bitcoin/pull/32953#discussion_r2204607453)
It seems fine to wait a few months until the 26.04 tag is available and then just use that. Also, it seems fine to just require cmake 3.31 (or higher) if anyone wants to use the codegen target, but no strong opinion.

💬 fanquake 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-3069047625)
https://github.com/bitcoin/bitcoin/actions/runs/16263971992/job/45915380111#step:13:114

💬 Sjors commented on pull request "descriptor: don't underestimate the size of a Taproot spend (instead, overestimate it)":
(https://github.com/bitcoin/bitcoin/pull/32964#issuecomment-3069141228)
I don't think we should be overpaying by default, for two reasons:
1. it's expensive
2. it reveals the presence of script paths

An intermediate solution could be that we never sign script paths by default, i.e. make `keypath_only` the default after #32857. Only when the user opts in to script path spending do we apply this solution of using the worst case fee. Since at least that's better than the current "solution" of manually picking a higher fee rate.