👍 stickies-v approved a pull request: "http: Track active requests and wait for last to finish - 2nd attempt"
(https://github.com/bitcoin/bitcoin/pull/26742)
(https://github.com/bitcoin/bitcoin/pull/26742)
💬 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_r1103622002)
ah yeah, good catch. All the numbers are in weight units. And yes, inputs are all P2WPKH (all of them in every `coinselector_tests.cpp` test case).
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103622002)
ah yeah, good catch. All the numbers are in weight units. And yes, inputs are all P2WPKH (all of them in every `coinselector_tests.cpp` test case).
💬 Sjors commented on pull request "mempool: Increase mempool default size":
(https://github.com/bitcoin/bitcoin/pull/27079#issuecomment-1426760411)
I believe there was no mempool limit until late 2015, a 300 MB limit was added in 794a8cec5db84fde1cce82ada51740070ec188ac.
With the recent influx of relatively large blocks it still fit 70 blocks worth of transactions. Assuming 50% overhead in the way we store transactions in the mempool, I think the worst case is 40 blocks, or about 6 hours. This is significantly less than the default expiration of mempool transactions, which is two weeks. Before SegWit, our mempool would fit 1 day of wors
...
(https://github.com/bitcoin/bitcoin/pull/27079#issuecomment-1426760411)
I believe there was no mempool limit until late 2015, a 300 MB limit was added in 794a8cec5db84fde1cce82ada51740070ec188ac.
With the recent influx of relatively large blocks it still fit 70 blocks worth of transactions. Assuming 50% overhead in the way we store transactions in the mempool, I think the worst case is 40 blocks, or about 6 hours. This is significantly less than the default expiration of mempool transactions, which is two weeks. Before SegWit, our mempool would fit 1 day of wors
...
💬 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-1426761886)
I would prefer a solution that allows checking commits by retired maintainers, at least up to a few years back (e.g. the branch-off point for the last backport supported version).
(https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1426761886)
I would prefer a solution that allows checking commits by retired maintainers, at least up to a few years back (e.g. the branch-off point for the last backport supported version).
💬 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-1426763832)
You can do `git checkout "$(cat contrib/verify-commits/trusted-git-root)~1"` and then verify the previous commits, as often as you want, to go as far back as you want.
(https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1426763832)
You can do `git checkout "$(cat contrib/verify-commits/trusted-git-root)~1"` and then verify the previous commits, as often as you want, to go as far back as you want.
💬 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_r1103623941)
fixed
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103623941)
fixed
💬 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_r1103623954)
done
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103623954)
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_r1103623962)
done
(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
(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).
(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.
(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
...
(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
...
(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.
(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?
(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
...
(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.
(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
...
(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)
(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.
(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?
(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?