💬 maflcko commented on pull request "validation: improve checkblockindex comments":
(https://github.com/bitcoin/bitcoin/pull/29299#discussion_r1464467510)
```suggestion
// Transaction may be completely unset - happens if only the header was accepted but the block or it's ancestors haven't been downloaded.
```

nit: The block itself may have been processed (ProcessNewBlock is called). Maybe clarify that you meant that this block and all previous ones have been processed or downloaded to storage?

💬 maflcko commented on pull request "validation: improve checkblockindex comments":
(https://github.com/bitcoin/bitcoin/pull/29299#discussion_r1464471710)
```suggestion
// nChainTx may be unset, but nTx set (if a block has been accepted, but one of its predecessors hasn't been processed yet)
|| (pindex->nChainTx == 0 && pindex->nTx >=1 && prev_chain_tx == 0 && pindex->pprev)
```

nit: Missing nTx check, according to comment?

💬 maflcko commented on issue "Build error on macOS, tag v26.0 builds fine":
(https://github.com/bitcoin/bitcoin/issues/29289#issuecomment-1907608162)
> I think the conclusion is that I'm on clang 13 and need to upgrade. I'll close this issue (I don't have the will power to upgrade Xcode/clang on that machine right now and verify, but if it doesn't resolve the issue when I get to this I'll report back)

I think you are on clang 12 (for some reason apple calls clang 12 clang1300), according to https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_%28since_SwiftUI_framework%29

But yeah, let us know if this is still an issue after upgrading
...

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464497339)
I think it deserves a comment :)

💬 maflcko commented on pull request "ci: Rename tasks (previous releases, macOS cross)":
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1907627949)
> > a new label can be added, if a dedicated "category" is needed
>
> I think this would be great. Ideally there would just be some link you could go to quickly see recent known CI failures and what the expected error messages look like.

Ok, I've recycled the "CI failed" label for this. See https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue+is%3Aopen+label%3A%22CI+failed%22

There are 13 issues right now. (14 if someone creates an issue for the one mentioned in this thread)

If the
...

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464515415)
The goal of this class is mostly to test the tests, right? Commenting that more clearly could help.

💬 S3RK commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464525381)
suggestion, add

```cpp
if (group.m_weight <= max_input_weight) continue;
```
on top of the for loop instead of checking within each condition.

Currently, you add groups that are above weight to `applicable_groups`

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1907670816)
ACK fa4d9d5b39d5652de1d83564f19b1a0749863b16

There are many more tests possible, right? E.g. sending more than 4095 bytes of garbage.
I assume the goal here wasn't to cover everything?

✅ maflcko closed an issue: "Win64 CI failure in feature_versionbits_warning.py"
(https://github.com/bitcoin/bitcoin/issues/28115)

💬 maflcko commented on issue "Win64 CI failure in feature_versionbits_warning.py":
(https://github.com/bitcoin/bitcoin/issues/28115#issuecomment-1907723845)
Is this still an issue with a recent version of Bitcoin Core? Closing for now, but feel free to reopen if this happens again.

✅ maflcko closed an issue: "test: failure in feature_notifications.py (PermissionError: [WinError 32] The process cannot access the file because it is being used by another process:)"
(https://github.com/bitcoin/bitcoin/issues/27352)

💬 maflcko commented on issue "test: failure in feature_notifications.py (PermissionError: [WinError 32] The process cannot access the file because it is being used by another process:)":
(https://github.com/bitcoin/bitcoin/issues/27352#issuecomment-1907727054)
Is this still an issue with a recent version of Bitcoin Core? Closing for now, but feel free to reopen if this happens again.

💬 maflcko commented on issue "ci: feature_proxy failing in MSVC job":
(https://github.com/bitcoin/bitcoin/issues/29090#issuecomment-1907735929)
> Similar failure, different test: https://github.com/bitcoin/bitcoin/actions/runs/7249043694/job/19746211856#step:27:521.

Not sure. This one has

```
Error: 2023-12-18T14:15:31.391534Z [D:\a\bitcoin\bitcoin\src\logging.h:264] [error] ERROR: Commit: Failed to commit latest txindex state
```

in the log.

👍 vasild approved a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1840919344)
ACK 27f260aa6e04f82dad78e9a06d58927546143a27

Wrt the interface addition discussed in https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1285990092, I think it is not perfect but I do not see it as a blocker for this PR. Would be nice to see the discussion resolved in one way or another.

💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464705680)
> > It does not check whether ferror(3) occurred.
>
> It does.

Where? There is no `ferror(3)` call. From the man page: "The function fread() does not distinguish between end-of-file and error, and callers must use feof(3) and ferror(3) to determine which occurred."

> If the return value of `detail_fread` is not `output.size()`, `operator>>` will fail.

Yes, but it can be equal to the desired size under two conditions: eof, or error. The previous code distinguished between both.

📝 dergoegge opened a pull request: "fuzz: Exit and log stdout for parse_test_list errors"
(https://github.com/bitcoin/bitcoin/pull/29304)
We should log all errors that occur when attempting to print the harness list in the fuzz test runner.

💬 dergoegge commented on pull request "fuzz: Exit and log stdout for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#issuecomment-1907892490)
This is currently a problem in the qa-assets CI, listing the harnesses fails but there is no output: https://cirrus-ci.com/task/5433841842651136?logs=ci#L9235

```
...
+ test/fuzz/test_runner.py -j6 -l DEBUG /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/ --empty_min_time=60
No fuzz targets found
```

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1464749909)
> > If the return value of `detail_fread` is not `output.size()`, `operator>>` will fail.
>
> Yes, but it can be equal to the desired size under two conditions: eof, or error.

No, the eof-error would only be raised if read *past* the desired size, not *to* it. Unless I am missing something?

I am asking, because if there was a bug, it should be fixed, or at least an issue should be filed.

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1464751330)
An alternative to the mocking would be to use [ `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`](https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode) in all places that call `CheckProofOfWork` (i.e. validation, mining) since there is literally zero benefit in having this be a blocker for fuzzing.

💬 maflcko commented on pull request "fuzz: Exit and log stdout for parse_test_list errors":
(https://github.com/bitcoin/bitcoin/pull/29304#discussion_r1464756961)
This will now include stderr, because it was redirected to stdout, no? Seems easier to keep them separate?