💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959586025)
The `Assume()` can be triggered during normal circumstances: if the first wait quits with `TipBlock()` null and `chainman().m_interrupt` being `true`. The easiest way to work around this would be to swap the order in the second condition:

```diff
- Assume(notifications().TipBlock()) != current_tip || chainman().m_interrupt
+ chainman().m_interrupt || Assume(notifications().TipBlock()) != current_tip
```
that might be a bit fragile though, wrt future changes.

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959608374)
Same here, if the user provides `height=0`, and `getTip()` returns `nullopt`, then this will be `while (0 < 0)`, will never enter and `block` will remain `nullopt` and be dereferenced at the end.

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959617648)
nit: maybe use `uint256::ZERO`, also used in commit b954af72e4d6a83b3ba701b7ee52d97f2af3254b `Have createNewBlock() wait for a tip`:

```suggestion
uint256 tip_hash{block ? block->hash : uint256::ZERO};
```

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959604318)
This could crash in the following (stupid) scenario:
* the user provides `0` as desired block hash to the `waitforblock`
* `getTip()` returns `nullopt`, which is assigned to `block`
* the `while` loop is never entered, not even once so `block` remains `nullopt`
* at the end we try to dereference the `nullopt` `block` :rescue_worker_helmet:

We shouldn't crash even on such "idiotic" input. Maybe change the `while` condition:

```diff
- while (tip_hash != hash) {
+ while (tip_hash != ha
...

👍 hebasto approved a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892#pullrequestreview-2623386934)
ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04.

> This check is only used in release builds...

Not directly related to this PR, but It seems reasonable to consider removing this condition as well:https://github.com/bitcoin/bitcoin/blob/28dec6c5f8bd35ef4e6cb8d7aa3f21b6879acf98/cmake/module/Maintenance.cmake#L27

👋 hebasto's pull request is ready for review: "cmake: Make implicit `libbitcoinkernel` dependencies explicit"
(https://github.com/bitcoin/bitcoin/pull/31884)

💬 hebasto commented on pull request "cmake: Make implicit `libbitcoinkernel` dependencies explicit":
(https://github.com/bitcoin/bitcoin/pull/31884#issuecomment-2665584719)
cc @TheCharlatan @theuni

🤔 rkrux reviewed a pull request: "descriptor: check whitespace in pubkeys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2623382128)
Somewhere either in the PR title or description, it should be mentioned that this works for descriptors containing private keys as well as observed in the tests and in the `ParsePubKeyInner` code: https://github.com/bitcoin/bitcoin/blob/28dec6c5f8bd35ef4e6cb8d7aa3f21b6879acf98/src/script/descriptor.cpp#L1540-L1550

💬 rkrux commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959643968)
Can add 2 more assertions: one where the middle key in a multi descriptor contains a whitespace, another that tests on a private key.

```python
assert_raises_rpc_error(-5,
"Multi: Key ' 03774ae7f858a9411e5ef4246b70c65aac5649980be5c17891bbec17895da008cb' is invalid due to whitespace",
self.nodes[0].getdescriptorinfo,
"wsh(multi(2,03a0434d9e47f3c86235477c7b1ae6ae5d3442d49b1943c2b752a68e2a
...

💬 rkrux commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959636120)
WIF private keys are passed in the second argument of `CheckUnparsable`, which accepts a public key `const std::string& pub` instead. The comment also mentions pubkeys instead of privkeys.

A simple swapping of the arguments doesn't work because the error of the second argument overwrites the error of the first argument making the `CheckUnparsable()` brittle.

💬 rkrux commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959639872)
`contains a whitespace at the beginning or the end of`

🤔 sipa reviewed a pull request: "descriptor: check whitespace in pubkeys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2623448041)
utACK bdc6ac10caf92901657b4d95bf2c11cccc8ac9ea

It's unfortunate that this in theory breaks RPC importing for (very limited) positions where spaces were allowed before, but it makes sense for consistency. Given that wallets always store descriptors in normalized form, I don't believe this will result in wallet loading breaking.

👍 l0rinc approved a pull request: "doc: add missing copyright headers"
(https://github.com/bitcoin/bitcoin/pull/31864#pullrequestreview-2623350599)
While I admit having stronger feelings against the [copyright headers](https://github.com/bitcoin/bitcoin/issues/29445) than I should (noise + bikeshedding + revoking right to copy in general), I do value consistency, so it's a 👍 from me - I wish we could unify more to get rid of this extra source of disagreement, but this PR makes it a bit better, so:

ACK 01b9a6183eb58dffac00f053e2742d924c84c721

💬 l0rinc commented on pull request "doc: add missing copyright headers":
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1959618226)
👍 was removed in https://github.com/bitcoin/bitcoin/commit/3ed772d2219e58d6afea3d12c0ebebb8487445e7

💬 l0rinc commented on pull request "doc: add missing copyright headers":
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1959683707)
Only found these in Python dependencies, so: 👍

Unrelated: we could update the script to change the end year to `present` instead

💬 l0rinc commented on pull request "doc: add missing copyright headers":
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1959663069)
How were these files chosen for update?
I see that many other headers are missing the `-present` part:
* https://github.com/bitcoin/bitcoin/blob/01b9a6183eb58dffac00f053e2742d924c84c721/src/node/abort.cpp#L1
* https://github.com/bitcoin/bitcoin/blob/01b9a6183eb58dffac00f053e2742d924c84c721/src/common/args.h#L1
* https://github.com/bitcoin/bitcoin/blob/01b9a6183eb58dffac00f053e2742d924c84c721/src/bech32.h#L2

and many others have a older end year:
* https://github.com/bitcoin/bitcoin/blob/
...

💬 purpleKarrot commented on pull request "cmake: Make implicit `libbitcoinkernel` dependencies explicit":
(https://github.com/bitcoin/bitcoin/pull/31884#issuecomment-2665638833)
ACK. I agree that this approach is preferred over the mentioned alternative.

👍 TheCharlatan approved a pull request: "cmake: Make implicit `libbitcoinkernel` dependencies explicit"
(https://github.com/bitcoin/bitcoin/pull/31884#pullrequestreview-2623494959)
ACK 3b42e05aa9e38ba00d52b2338375b4caf032f041

I prefer this over your mentioned alternative too.

💬 TheCharlatan commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2665666986)
Thought a guix build might be prudent since we are changing a bunch of paths (built on aarch64):
```
b683fabbea0455a7f8a5c0566079b1bbce438edd5870e11209ce2dd5a016541e guix-build-186680acef66/output/aarch64-linux-gnu/SHA256SUMS.part
4c144bdb301bf7d81dd99a8cef2082950fd5912f8edba6cea545482445cf7dba guix-build-186680acef66/output/aarch64-linux-gnu/bitcoin-186680acef66-aarch64-linux-gnu-debug.tar.gz
c4e4a24a847dd420cd1b9310e13aef1e6376d069c5099f799918dd3f2c3a01e3 guix-build-186680acef66/output/
...

💬 jsarenik commented on issue "Memory-only wallet (for faucets)":
(https://github.com/bitcoin/bitcoin/issues/31816#issuecomment-2665671154)
@maflcko just for the record, the current wallet of alt.signetfaucet.com is running in Linux' `tmpfs` (i.e. in RAM) and is being refreshed by `cron` every hour. No leftovers. Actually I think there is no need for the proposed feature.

Thank you! I like Bitcoin Core!