💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103623962)
done

💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103623989)
done

💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#issuecomment-1426764558)
Feedback tackled, thanks for the review Xekyo!
Pushed [diff](https://github.com/bitcoin/bitcoin/compare/f0add1d0a7507e5601d7da54f62fe9ab272b3281..7d5a02ecb2af934ea0fa76c9013b2a2fc968b726).

💬 Sjors commented on pull request "verify-commits: Bump trusted git root to after most recent laanwj merge":
(https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1426765029)
That's a good hint to add to the `README`. But having the root key not too recent still seems better, all things equal. I think we should only bump it if a former maintainer revokes their key or it expires, plus maybe every couple of years to preempt such issues. So far #27058 seems compatible with that approach.

💬 darosior commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#issuecomment-1426765831)
@instagibbs thanks for the suggestions and the examples. This functional test was clearly lacking imagination. :) However what you suggest is already thoroughly tested by the unit tests (and to a lesser extent by the fuzz tests). But it's inexpensive to test also in the functional test so i included your examples and added a couple more things to test for each descriptor: the number of signatures expected (always equal to the number of private keys we have in the descriptor), whether this "compl
...

💬 darosior commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1103625462)
Initially i wanted to tackle all the PSBT integration in a followup (see https://github.com/bitcoin/bitcoin/pull/24149#issuecomment-1046944178). This comment made me (finally) have a look at what should be done. I think `analyzepsbt`, `utxoupdatepsbt` would need to be updated to support hash preimages. Maybe `walletprocesspsbt` too, but i'm not sure yet (for instance hash preimages could be imported separately and kept in a signing provider?). However i think we should first make those commands
...

💬 darosior commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1103625470)
Done. I added a refactoring commit to update the watchonly descriptors, and then use the keys list in the signing test too.

💬 MarcoFalke commented on pull request "verify-commits: Bump trusted git root to after most recent laanwj merge":
(https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1426768304)
> I think we should only bump

Is there any reason for this? See also https://github.com/bitcoin/bitcoin/pull/27058#discussion_r1099900493
What would the alternative be? Listing hundreds or thousands of "revsig" commits in a file, to ensure it is impossible to review manually, only with special git commands, potentially making it trivial to sneak in malicious commits that are not actually revsig commits?

💬 Sjors commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1426770715)
I like the CSV approach in c090a7ff86257411761e22d3642d0e8e05859b73 better. See [inline discussion](https://github.com/bitcoin/bitcoin/pull/27058#discussion_r1099900493) and [here](https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1426761886). I don't like having to update the root trusted commit every time the maintainer list changes.

Imo we should only bump it if a former maintainer revokes their key or it expires, plus maybe every couple of years to preempt such issues. As @MarcoF
...

💬 Sjors commented on pull request "verify-commits: Bump trusted git root to after most recent laanwj merge":
(https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1426770956)
@MarcoFalke answered in https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1426770715. Depending on what we do there this PR can either be closed or merged.

💬 MarcoFalke commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1426774872)
> I don't like having to update the root trusted commit every time the maintainer list changes.

You don't have to. It is only a recommended trusted git root. Everyone using this script will have to pick their root themselves in their own copy of this script anyway. They are free to not touch their script after the root is bumped and continue using the previous script+data; they are free to pick the latest data from the `master` branch, once and if they agree with it; they are also free to pi
...

👋 hebasto's pull request is ready for review: "ci: A few fixes of `ccache` issues"
(https://github.com/bitcoin/bitcoin/pull/27084)

💬 hebasto commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1426800453)
The PR description has been updated with some statistics.

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1103646477)
Hmm good point. Unfortunately this is a bit clumsy for the `LegacyScriptPubKeyMan`: since it's not a `FlatSigningProvider` it involves copying all its data in order to merge both providers. I've pushed the change in a separate commit to be squashed if it's not too unreasonable. What do you think?

⚠️ Christewart opened an issue: "Failing to fetch `cfheader` corresponding to block header in `headers` message"
(https://github.com/bitcoin/bitcoin/issues/27085)
<!-- Describe the issue -->

Occasionally when I receive a `headers` message on the p2p network, and attempt to fetch the `cfheader` that corresponds to a block header inside of a `headers`, i get this error message inside of my `debug.log`

>2023-02-07T16:47:15Z [net] Failed to find block filter hashes in index: filter_type=basic, start_height=150, stop_hash=3ca9030aeb6c2721cfbab0116b8d96e2d3c7e00738238010e0bc622566dc2aed


**Expected behavior**

I should be able to fetch a `cfheader`
...

💬 achow101 commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1426816110)
> But doesn't the CSV remove the need for revsig commits?

It could. The CSV could have a field that indicates the key is revoked, so we could then allow revsigs only for that key. Similar with expired keys.

💬 kristapsk commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1426821483)
Maybe it would make sense to use OpenTimestamps to verify timestamps (dates) of merge commits?

💬 furszy commented on issue "Failing to fetch `cfheader` corresponding to block header in `headers` message":
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1426836560)
Even when the peer sent the `headers` message, the block filter index could still be processing the block signals.
If possible on your tests, call `syncwithvalidationinterfacequeue` to empty the validation interface queue prior to request the `cfheader`s.

💬 Christewart commented on issue "Failing to fetch `cfheader` corresponding to block header in `headers` message":
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1426845565)
Thanks for pointing me in the right direction, this seems to be the equivalent in the bitcoin core python test harness https://github.com/bitcoin/bitcoin/pull/22311

✅ Christewart closed an issue: "Failing to fetch `cfheader` corresponding to block header in `headers` message"
(https://github.com/bitcoin/bitcoin/issues/27085)