💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-1926582804)
> > This class could `Assume` that the file was flushed/closed before the destructor is called?
>
> `Assume` is more for code correctness, not for external errors (like IO error).

The assumption would be that the close was called *before* the destructor is called. This is a code correctness question and seems like the perfect fit for Assume. That is, if the Assume fails, the code was missing a close, and the code must be changed to fix the internal bug. The Assume does not in any way check
...

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1477927227)
> compare it with the resulting mining score instead of the new transaction’s individual feerate?

🤔 Not sure how to implement the change you suggested honestly.
The current behavour is checking if target fee rate of user wants to bump the transaction to is greater than `maxfeerate` which I believe is the new mining score of the bumped transaction no?
I might be wrong on my assumption though, would you clarify your comment please? thanks.

⚠️ hebasto opened an issue: "build: `CPPFLAGS` usage in OSS-Fuzz"
(https://github.com/bitcoin/bitcoin/issues/29385)
In OSS-Fuzz, dependencies are built with `DEBUG=1`, which implies usage of the additional preprocessor definitions:

```
$ make -C depends print-host_debug_CPPFLAGS HOST=x86_64-pc-linux-gnu DEBUG=1
...
host_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_ENABLE_DEBUG_MODE=1
```

However, explicit setting `CPPFLAGS` breaks this assumption:
```
$ make -C depends print-host_debug_CPPFLAGS HOST=x86_64-pc-linux-gnu DEBUG=1 CPPFLAGS="-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"
...

💬 maflcko commented on issue "build: `CPPFLAGS` usage in OSS-Fuzz":
(https://github.com/bitcoin/bitcoin/issues/29385#issuecomment-1926623041)
Reference: https://github.com/google/oss-fuzz/blob/c801e7aab5822e0c305fdb88dbe21b834d45afea/projects/bitcoin-core/build.sh#L38

💬 maflcko commented on issue "build: `CPPFLAGS` usage in OSS-Fuzz":
(https://github.com/bitcoin/bitcoin/issues/29385#issuecomment-1926627045)
Same in `ci/test/00_setup_env_native_fuzz_with_msan.sh`?

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1926654400)
I rebased on top of #29346 and pushed the result to: https://github.com/Sjors/bitcoin/tree/2024/02/sv2-poll-ellswift

Once EllSwift lands on the SRI side (or if I make a revert commit for it) I'll update this PR.

👋 maflcko's pull request is ready for review: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369)

💬 maflcko commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1926712573)
Removed unused `value_type`, which was wrong anyway, since it was cv qualified when it shouldn't.

👍 maflcko approved a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1862394105)
lgtm

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998490)
Any reason to change the sequence id? I think this isn't changed in `loadtxoutset` either?

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477981888)
Any reason to touch this LOC?

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998941)
Same (sequence id)

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477996772)
```suggestion
index->nChainTx = 0;
```

isn't this always true? Maybe you meant to type `-` instead of `<`?

In any case, the test seems to be passing either way, so this is "dead" code, similar to a code comment.

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478018977)
Can this be further limited to only mark the snap_block if it has ASSUMED_VALID set? Otherwise, nodes which never used loadtxoutset will assume that some block is a "snapshot block", even though they never used that feature.

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478006900)
Is removing the validity check intended?

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478022794)
Any reason to remove this check?

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1926747949)
> so i took time to test a NTA pinning scenario: [ariard/bitcoin@84e12b8](https://github.com/ariard/bitcoin/commit/84e12b8) which is known since https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-June/018011.html

To summarize the "network topology aware" scenario, what happens with Alice and Bob's channel is:
1. Bob broadcasts his commitment transaction + cpfp from his anchor
2. This transaction propagates to everyone except Alice, somehow. [1]
3. Alice broadcasts her commitment
...

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1926771908)
Thanks Luke, I agree that repeated string concatenation was not a particularly neat for creating a single string where we could simply modify in place. I've updated this further from your version to use a lambda in bf9b11d8f6c0af9b2c76674481e5905a00240cf5.

Re Windows. Yes, fs::permissions is "supported", but IIUC without devolving into access control lists, it all boils down to whether the write bit is enabled. I don't see the sense in supporting either of these things here personally. The de
...

💬 delta1 commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1926795738)
ACK [fa9ef95](https://github.com/bitcoin/bitcoin/pull/29369/commits/fa9ef95b4c1114e2d8f7c3307fdf01446efee6fd)

💬 maflcko commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1926850992)
I guess this can be fixed by using std::string_view::starts_with

💬 maflcko commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1926859778)
I've created https://github.com/bitcoin/bitcoin/issues/26972 to track ideas on how to detect those silent build issues, or at least make them easier to spot.