💬 fanquake commented on pull request "Remove Autotools packages from depends and CI":
(https://github.com/bitcoin/bitcoin/pull/30902#issuecomment-2352582657)
> About to close it in favour of https://github.com/bitcoin/bitcoin/pull/30875.

Mark as draft for now, or close?

💬 maflcko commented on pull request "Remove Autotools packages from depends and CI":
(https://github.com/bitcoin/bitcoin/pull/30902#issuecomment-2352585712)
I think it is fine to do the CI/libtool changes separate. Looks like the other pull is still a draft anyway.

🤔 pablomartin4btc reviewed a pull request: "doc: cmake: prepend "build" to functional/test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2306330141)
ACK 15cc65649b78ffcbd1e1b302c2e4866623cee294

> (Bonus points if the message can be defined in one place, didn't check.)

I agree. `ConfigParser` should not raise an exception if the config file path is invalid?

💬 fanquake commented on pull request "cmake: Switch to crc32c upstream build system":
(https://github.com/bitcoin/bitcoin/pull/30905#issuecomment-2352607141)
> A few things still need to be addressed:

This PR is also changing the flags used for code generation. Is that intentional? If yes, would be good to mention in the PR description (and why).

💬 maflcko commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2352609290)
Overall, I think when adding a new feature (even if it is "just" CI code), there should be a reason and motivation, along with some evidence that it will be used correctly and meaningfully.

Looking at the comments so far, it looks like the motivation is weak at best, there doesn't seem to be any evidence of a (useful) use-case in any past commit, and it could even promote incorrect usage, leading to issues down the line.

👍 stickies-v approved a pull request: "tidy: add clang-tidy `modernize-use-starts-ends-with` check"
(https://github.com/bitcoin/bitcoin/pull/30868#pullrequestreview-2306337434)
ACK fc7b507e9a595a7bf91f4e0f42b4d842af8b93f3

👍 hebasto approved a pull request: "refactor: add clang-tidy `modernize-use-starts-ends-with` check"
(https://github.com/bitcoin/bitcoin/pull/30868#pullrequestreview-2306358634)
ACK fc7b507e9a595a7bf91f4e0f42b4d842af8b93f3, I have reviewed the code and it looks OK.

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2352637642)
Sorry for not providing an update earlier. But yes, I continue working on this. I am trying to debug the CI issues with no success so far.

I mounted a virtual machine with ubuntu 22.04 (8GB ram), and after dealing with some libraries and memory issues I was able to run the CI job [as described here ](https://github.com/bitcoin/bitcoin/blob/master/ci/README.md).

However the jobs takes around 4-5 hours to run in the VM and I was not able to reproduce the issue yet. I was using the root user
...

💬 Sjors commented on pull request "Remove Autotools packages from depends and CI":
(https://github.com/bitcoin/bitcoin/pull/30902#issuecomment-2352657327)
Maybe rename the PR to: "Remove Autotools packages from CI (and depends doc)"

💬 hebasto commented on pull request "Remove Autotools packages from CI (and depends doc)":
(https://github.com/bitcoin/bitcoin/pull/30902#issuecomment-2352663089)
> Maybe rename the PR to: "Remove Autotools packages from CI (and depends doc)"

Renamed.

💬 maflcko commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2352665318)
Ugh, the error is https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535

```
node1 stderr ./src/validation.cpp:5569 GuessVerificationProgress: Assertion `pindex->m_chain_tx_count > 0' failed. `

```

This means that this is a real issue that should be fixed in the real code. This test is just surfacing it.

💬 brunoerg commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2352710746)
> The assert in the harness should fail with that input after reverting https://github.com/bitcoin/bitcoin/commit/7ad15d11005eac36421398530da127333d87ea80

Yes, confirmed!

```sh
Assertion failed: (WITH_LOCK(cs_main, return chainman.m_blockman.m_block_index.size()) == original_index_size), function p2p_headers_presync_fuzz_target, file p2p_headers_presync.cpp, line 197.
==80161== ERROR: libFuzzer: deadly signal
#0 0x107edce84 in __sanitizer_print_stack_trace+0x28 (libclang_rt.asan_os
...

🤔 pablomartin4btc reviewed a pull request: "rpc: Add support to populate PSBT input utxos via rpc"
(https://github.com/bitcoin/bitcoin/pull/30886#pullrequestreview-2306505506)
ACK abc835282a66495f65f342cfc113e45aac4ebb48

> 1. Was this functionality already exposed somewhere I missed?

I couldn't find anything but perhaps I missed some.

> 2. Should "non-witness utxos" always be the type added when updating the PSBT? Looks like we fill in non-witness wherever we can, and witness whenever scraping from utxo set

You mean that segwit txs would need to be converted into legacy ones to be passed in `prev_txs`?

I've checked the test and I understand the use cas
...

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2352802388)
> Ugh, the error is https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535
>
> ```
> node1 stderr ./src/validation.cpp:5569 GuessVerificationProgress: Assertion `pindex->m_chain_tx_count > 0' failed. `
> ```
>
> This means that this is a real issue that should be fixed in the real code. This test is just surfacing it.

Thanks for pointing this out. I hadn't noticed the assertion was failing.

It seems the assertion was [introduced here](https://github.com/bitcoin/bitcoin/pull/296
...

💬 kevkevinpal commented on pull request "Remove Autotools packages from CI (and depends doc)":
(https://github.com/bitcoin/bitcoin/pull/30902#issuecomment-2352835417)
> I think it is fine to do the CI/libtool changes separate. Looks like the other pull is still a draft/WIP anyway.

yup agreed I can remove overlapping changes from this PR and https://github.com/bitcoin/bitcoin/pull/30875

💬 kevkevinpal commented on pull request "Remove Autotools packages from CI (and depends doc)":
(https://github.com/bitcoin/bitcoin/pull/30902#issuecomment-2352836170)
ACK [7a8a6a0](https://github.com/bitcoin/bitcoin/pull/30902/commits/7a8a6a06676dcb4066cc81a4e6872281a09bb00d)

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change and removed libtools from CI":
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2352848193)
> @kevkevinpal
>
> I didn't notice that #30902 overlaps with this PR. Feel free to pick all changes from #30902 to let me close it.
>
> UPD. Please separate CI changes into a dedicated commit with the `ci: ` prefix.

We can keep your PR for now and I just removed any overlapping changes in [9e4f9c9](https://github.com/bitcoin/bitcoin/pull/30875/commits/9e4f9c98e30985a5033bcb69d97adcd7f858adb1)

💬 instagibbs commented on pull request "rpc: Add support to populate PSBT input utxos via rpc":
(https://github.com/bitcoin/bitcoin/pull/30886#issuecomment-2352905099)
@pablomartin4btc I expanded on the use-case a bit in the OP

For (2) I was asking what was typically done for `non_witness_utxo` vs `witness_utxo`. Due to pre-taproot weaknesses in sighash format I think it's typical to add the full transaction when possible in this codebase at least, and I conform to that in this PR.

💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1761208862)
I think we should target the expected scenario, not the failure, in both cases because that's how tests will usually execute.
`ensure_for` plays the opposite role of `wait_until`. We want to verify that a certain condition does not hold for a period of time (it is the timeout condition of `wait_until`). And, in fact, we could implement it using the `wait_until` function too (quick and dirty - not saying to do it in this way):
```python3
def ensure_for(self, wait_until_negated_condition, durat
...

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1761212459)
would be worth adding that description if touched again or in followup