💬 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.)

💬 jonatack commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2353137979)
If I understand correctly, reviewers (or possibly DrahtBot) should signal if `[skip ci]` is being used inappropriately in any case.

If skipping it is considered appropriate for certain changes (e.g. markdown files as mentioned above) to save time and compute resources, then this pull makes sense to me.

If skipping it is discouraged in all cases, then yes, no need for this update -- in which case it may be useful for the bot, if feasible, to signal any case it detects that `[skip ci]` is us
...

💬 l0rinc commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1761310952)
this doesn't seem to be the case anymore, the above ones recreate the folders

💬 maflcko commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1761324831)
I meant the "bind-mount" part, not the folder creation. (I think creating the folder is best required to be done outside the script, as permission bits need to be set appropriately)

I guess I can just remove the comment?