👍 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

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1761219998)
> And, in fact, we could implement it using the wait_until function too

Yes, I tried this and decided against it, see the description of the PR.

> I’d suggest simply waiting for the entire duration without intermediate checks.

I have also considered this and decided against it because I don't think it's better. I have also described this in the PR description already. Since the polling doesn't seem to lead to a real performance issue I think it's better to fail faster if we fail. This a
...

💬 maflcko commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1761224717)
> So, I’d suggest simply waiting for the entire duration without intermediate checks.

I'd say doing the check once more at the beginning should help, to catch misuse of the function, where a predicate is expected to trigger after some time.

(I think this may allow to remove `time.sleep` from within tests completely, because all code could be changed to use either (1) `ensure_for`, (2) mocktime, or (3) `wait_until`?)

💬 jonatack commented on pull request "refactor: add clang-tidy `modernize-use-starts-ends-with` check":
(https://github.com/bitcoin/bitcoin/pull/30868#issuecomment-2353024807)
re-ACK fc7b507e9a595a7bf91f4e0f42b4d842af8b93f3 only change since my previous ACK is the commit message

🤔 jonatack reviewed a pull request: "Remove Autotools packages from CI (and depends doc)"
(https://github.com/bitcoin/bitcoin/pull/30902#pullrequestreview-2306847826)
LGTM at 7a8a6a06676dcb4066cc81a4e6872281a09bb00d, this patch appears to correctly remove the packages that can be dropped while leaving a few remaining ones that are unrelated.

The changes are small enough that I found it easier to review them together and could be squashed.

💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2353094719)
(DrahtBot is pinging for a re-review, but the pull hasn't been updated yet.)