💬 mmgen commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1851389569)
The Bitcoin whitepaper is embedded in the blockchain in a [transaction](https://blockstream.info/tx/54e48e5f5c656b26c3bca14a8c95aa583d07ebe84dde3b7dd4a78f4e4186e713) with 946 1-satoshi multisig outputs.

What’s to prevent any arbitrary data from being embedded in this way, and in other ways this PR doesn’t address?

And at the risk of belaboring the obvious, I'll repeat: any mempool-based approach to filter transactions is futile, because transactions don’t require the mempool to get into a
...

💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1851494243)
On Mon, Dec 11, 2023 at 10:39:42PM -0800, The MMGen Project wrote:
> The Bitcoin whitepaper is embedded in the blockchain in a [transaction](https://blockstream.info/tx/54e48e5f5c656b26c3bca14a8c95aa583d07ebe84dde3b7dd4a78f4e4186e713) with 946 1-satoshi multisig outputs.
>
> What’s to prevent any arbitrary data from being embedded in this way, and in other ways this PR doesn’t address?

It doesn't even stop https://github.com/petertodd/python-bitcoinlib/blob/master/examples/publish-text.py, whi
...

💬 S3RK commented on pull request "tests, bench: Fix issue with CWallet::LoadWallet() being called in the wrong places":
(https://github.com/bitcoin/bitcoin/pull/29055#issuecomment-1851504017)
ACK bd7f5d33e3b9e01cd600afe1682b7afa935f68a3

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1851506064)
Re https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1850970368

> But what else do you want beyond that? And how does removing sock.cpp from the util library help, for example?

Maybe removing `sock.cpp` is a bad example, since it is already impossible to link against it. `sock.cpp` uses modules that are already not part of the kernel, so trying to link it in will result in errors. Then again, if `sock.cpp` uses common modules, why is it in `util` in the first place? To provide ano
...

💬 hebasto commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1851535346)
Concept ACK.

💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1423659099)
> This has nothing to do with C++20

ok, rephrase (I mentioned C++20 because C++17 does not have `char8_t`):

What would be an example where it is broken by calling `string()` (1.) and ok by converting `wchar_t`->`char8_t`->`char` (2.2)?

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1851561925)
Looking at the diff between the two versions, I see these changes:
1. which peers are taken into account (`FEELER`, `ADDRFETCH`, `BLOCKRELAY` are also considered)
2. the 199-bug is brought back
3. fixed addition overflow; an alternative would divide each element by 2 and then add without overflow risk, it saves us the sloppiness `int64_max/2`; but i don't care that much)

Not sure i see `keeps warning condition for a large median time offset as it is on master`.

Generally, while I think
...

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851608976)
ACK https://github.com/bitcoin/bitcoin/commit/576bee88fd36e207b7288077626947a1fce0fc33

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851611159)
> ACK [18669d7](https://github.com/bitcoin/bitcoin/commit/18669d753a15f997f7363dcaf0abf230b851b224)

guessing you meant to ack https://github.com/bitcoin/bitcoin/commit/576bee88fd36e207b7288077626947a1fce0fc33 ? :wink:

💬 hebasto commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1851613999)
> ... but I wanted to allow our tests/fuzzers to run with this change first to help convince reviewers that `std::bit_width` is a drop-in replacement for`CountBits`.

I'm convinced :)

💬 naumenkogs commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1851617327)
Concept ACK

👍 maflcko approved a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1777064592)
It would be good to explain why the behavior changes are needed. Otherwise,

concept ACK eaf915d61d470372e63f41f11d6a873c1936f73f 👇

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

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423755089)
nit in eaf915d61d470372e63f41f11d6a873c1936f73f: Can drop the `()` around `Assert()`?

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423738990)
same

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423753937)
nit in eaf915d61d470372e63f41f11d6a873c1936f73f: "*the* main thread"

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423738157)
ae6642973dead188514aceec30ae5d73404ebcdf: Is it required to change the behavior here? Why not keep the abort?

📝 hebasto opened a pull request: "Revert "ci: Only run functional tests on windows in master""
(https://github.com/bitcoin/bitcoin/pull/29059)
This PR reverts commit aba4a5887b44bf7cbee9ea0a8e02bb92c1b4147b from https://github.com/bitcoin/bitcoin/pull/28567.

The Windows-specific code received [quality](https://github.com/bitcoin/bitcoin/pull/28486) and [performance](https://github.com/bitcoin/bitcoin/pull/29045) improvements recently. So there are no reasons to skip functional tests in PRs anymore.

In my own repo, I've run the GHA Windows job more than 100 times with no failure.

💬 maflcko commented on pull request "Revert "ci: Only run functional tests on windows in master"":
(https://github.com/bitcoin/bitcoin/pull/29059#discussion_r1423789241)
How much longer would it take to also run `--extended`?

💬 hebasto commented on pull request "Revert "ci: Only run functional tests on windows in master"":
(https://github.com/bitcoin/bitcoin/pull/29059#discussion_r1423803255)
> How much longer would it take to also run `--extended`?

Appr. 10 minutes.

The `feature_dbcrash.py` test takes the longest time.

🚀 fanquake merged a pull request: "refactor: rpc: Pass CBlockIndex by reference instead of pointer"
(https://github.com/bitcoin/bitcoin/pull/29021)

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1851803284)
Removing adjusted time from consensus is a big win. Having time samples received from peers be involved in consensus checks is not great and not effective against NTP based attacks (https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1264632475, https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1271239626). The warning itself is not as interesting to me but most of the review on both #25908 and this PR is about the warning and its threshold.

I don't want changing the warning (
...