💬 maflcko commented on pull request "AssumeUTXO follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1455739230)
Turned into an issue: https://github.com/bitcoin/bitcoin/issues/29261

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455749485)
I'm also not sure what to do with this one. We seem to be fine with not doing nullptr checks for very similar code, e.g. `base.cpp:89`, so I did not include any checks here. Maybe it is better to add a reference to the signals to `BaseIndex`?

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455760981)
> We seem to be fine with not doing nullptr checks for very similar code, e.g. base.cpp:89

That isn't run during shutdown, but during start, so the code path should be exercised in tests. Also, it is only "temporary", so will be removed in any case. Whereas the code here is run during shutdown, for which it is hard (impossible?) to test all code path permutations. Also it isn't temporary, is it?

👍 willcl-ark approved a pull request: "refactor: Allow std::span construction from CKey"
(https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1827516229)
ACK fa96d937116682f32613d31a3ae7d6f652e8146d

A nice simple change to enable using `std::span` with `CKey`.

The PR description could be updated now that the verification commit has been removed.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455808582)
Right, not temporary, and in the `Stop` case we should guard against early destruction of the signals in the `Shutdown` procedure. I'll add an `Assert` for this case.

🤔 mzumsande reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1827576365)
> Hmm, good point. I would slightly rephrase, and extend, your first paragraph to state that the net limited buffer is connected to a user-customizable parameter, `-maxtipage`, with its default value is set to 24h.

Yes, but `-maxtipage` is just an undocumented debug-only option meant for testing, nothing to be used by actual users on mainnet.

💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1455867835)
one more reference to `CheckForStaleTipAndEvictPeers` left

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1455897632)
fixed. Thanks

📝 maflcko opened a pull request: " rpc: Fix race in loadtxoutset "
(https://github.com/bitcoin/bitcoin/pull/29262)
The tip may have advanced, also if it did not, there is no reason to
have two variables point to the same block.

Fixes https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344694600

💬 real-or-random commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1896095271)
Hm, this is really not expected.

> Compiled with `gcc 10.2.1`.

You could see if gcc 10.5.0 makes a difference. AFAIU this is used in the official builds currently (until https://github.com/bitcoin/bitcoin/pull/27897 lands), and this was one of our considerations.

If that doesn't help, please benchmark libsecp256k1 with (https://github.com/bitcoin-core/secp256k1/commit/10e6d29b60c3931e327bc18e6c50cea78296b1ba) and without https://github.com/bitcoin-core/secp256k1/pull/1446 (https://githu
...

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1896099957)
> > Hmm, good point. I would slightly rephrase, and extend, your first paragraph to state that the net limited buffer is connected to a user-customizable parameter, `-maxtipage`, with its default value is set to 24h.
>
> Yes, but `-maxtipage` is just an undocumented debug-only option meant for testing, nothing to be changed by actual users on mainnet.

For sure. Nothing to be changed. It was just good to mention the possible issue.
Still, on a slightly connected note; you would be surprise
...

💬 vasild commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1896105759)
This tests extension was stalled because it uncovered a bug. Now the bug is fixed via https://github.com/bitcoin/bitcoin/pull/27071. @dergoegge, @mzumsande, @jonatack, @chinggg - you did Concept ACK, maybe you want to review it?

🚀 fanquake merged a pull request: "refactor: Allow std::span construction from CKey"
(https://github.com/bitcoin/bitcoin/pull/29133)

💬 tromp commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1896118061)
> they put text (link) in many cases

Yes, some do. But many others don't, and those are the ones these measures are targetting.

🚀 fanquake merged a pull request: "contrib: Update clang-format-diff"
(https://github.com/bitcoin/bitcoin/pull/29251)

💬 jamesob commented on pull request "rpc: Fix race in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29262#issuecomment-1896156373)
Approach ACK, thanks.

💬 TheCharlatan commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1896171001)
Guix builds:
```
c65c40452ea0c2485c8a17a4d79f4befd2f3f5b66af5281a7833842990a236be guix-build-00c1e2aa4496/output/aarch64-linux-gnu/SHA256SUMS.part
c027a5f50f2b767a251953eded0cef07adf932f4905e5bd6cb1b19fd013a9f8d guix-build-00c1e2aa4496/output/aarch64-linux-gnu/bitcoin-00c1e2aa4496-aarch64-linux-gnu-debug.tar.gz
2baa2e82b8af6171b7861e6487fb3c1a07f0b0637e1518fb047d9f0773922781 guix-build-00c1e2aa4496/output/aarch64-linux-gnu/bitcoin-00c1e2aa4496-aarch64-linux-gnu.tar.gz
7452fa6211244d19607
...

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1896179668)
5755454fb5cc4067fc94e2682116e0fc5c9dfc58 -> 487b12e18ce007379a997da48b5ee97f459f6342 ([noGlobalSignals_2](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_2) -> [noGlobalSignals_3](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_2..noGlobalSignals_3))

* Addressed @maflcko's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455548750), adding `Assert` to guard against early
...

👍 vasild approved a pull request: "fuzz: make FuzzedDataProvider usage deterministic"
(https://github.com/bitcoin/bitcoin/pull/29043#pullrequestreview-1827705262)
ACK 01960c53c7d71c70792abe19413315768dc2275a

I think it is worth improving this even if currently we only fuzz with clang and even if clang has not changed orders before or due to compilation flags - this may happen in the future.

A clang-tidy plugin to enforce this would be nice but I do not see its absence as a blocker for this PR.

💬 vasild commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#discussion_r1456045072)
Unrelated to this PR, out of scope, but passing `max_pct` greater than `100` is pointless. `AddrManImpl::GetAddr_()` will behave as if `100` is passed. In addition to being useless and confusing this skews the input towards `100`. That is, it will treat `> 97%` of the possibilities as `100` (`(4097-100)/4097*100 = 97.6%`).