š¬ 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)
(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.
(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
...
(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
(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
...
(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`?)
(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
(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.
(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.)
(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
...
(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
(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?
(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?
š¬ maflcko commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2353183613)
> 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.
I don't think it is appropriate to skip, even for markdown, see the reply above: https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2352219690
> 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 det
...
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2353183613)
> 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.
I don't think it is appropriate to skip, even for markdown, see the reply above: https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2352219690
> 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 det
...
š¬ jonatack commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2353192340)
> I don't think it is appropriate to skip, even for markdown, see the reply above: https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2352219690
> There are many other linters that do raise, and even many that only serve to lint docs. If they serve no purpose, they should be removed, instead of being (required to be) skipped.
Which linters only serve to lint documentation-only files? (I could be missing some.)
> Regardless, many doc updates are only checked at compile time (doxy
...
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2353192340)
> I don't think it is appropriate to skip, even for markdown, see the reply above: https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2352219690
> There are many other linters that do raise, and even many that only serve to lint docs. If they serve no purpose, they should be removed, instead of being (required to be) skipped.
Which linters only serve to lint documentation-only files? (I could be missing some.)
> Regardless, many doc updates are only checked at compile time (doxy
...
š¬ maflcko commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2353200196)
> (DrahtBot is pinging for a re-review, but the pull hasn't been updated yet.)
This is intentional, because at least one reviewer thinks that no further update is needed and indicated with their review that the change would be fine to merge. The Bot can't evaluate the content of the review comments itself (obviously), so it is up to each reviewer to evaluate whether it can be merged as-is, or not.
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2353200196)
> (DrahtBot is pinging for a re-review, but the pull hasn't been updated yet.)
This is intentional, because at least one reviewer thinks that no further update is needed and indicated with their review that the change would be fine to merge. The Bot can't evaluate the content of the review comments itself (obviously), so it is up to each reviewer to evaluate whether it can be merged as-is, or not.
š¬ sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1761351337)
Will do if I retouch.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1761351337)
Will do if I retouch.
š¬ jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2353234685)
Thanks @maflcko (hm, maybe bump the threshold to 2 new ACKs, or drop the rule in this case).
Friendly ping to @LarryRuane to update.
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2353234685)
Thanks @maflcko (hm, maybe bump the threshold to 2 new ACKs, or drop the rule in this case).
Friendly ping to @LarryRuane to update.
š¬ maflcko commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2353257205)
> Thanks @maflcko (hm, maybe bump the threshold to 2 new ACKs, or drop the rule in this case).
Pull requests, or issues are welcome, see also https://github.com/maflcko/DrahtBot/issues/33. However, in this case I'd find it useful to be pinged (otherwise I may miss/forget to re-review or otherwise reply).
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2353257205)
> Thanks @maflcko (hm, maybe bump the threshold to 2 new ACKs, or drop the rule in this case).
Pull requests, or issues are welcome, see also https://github.com/maflcko/DrahtBot/issues/33. However, in this case I'd find it useful to be pinged (otherwise I may miss/forget to re-review or otherwise reply).
š¬ fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2353260451)
> It seems the assertion was [introduced here](https://github.com/bitcoin/bitcoin/pull/29656/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R5556), and my branch doesn't include it.
No, that's just a commit where the variable is renamed. The `Assume` was introduced here: https://github.com/bitcoin/bitcoin/pull/29370/commits/0fd915ee6bef63bb360ccc5c039a3c11676c38e3. The [assumption (pun intended)](https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479427801)
...
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2353260451)
> It seems the assertion was [introduced here](https://github.com/bitcoin/bitcoin/pull/29656/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R5556), and my branch doesn't include it.
No, that's just a commit where the variable is renamed. The `Assume` was introduced here: https://github.com/bitcoin/bitcoin/pull/29370/commits/0fd915ee6bef63bb360ccc5c039a3c11676c38e3. The [assumption (pun intended)](https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479427801)
...
š¬ hebasto commented on pull request "build: improve cxx and linker flag caching":
(https://github.com/bitcoin/bitcoin/pull/30732#issuecomment-2353274116)
Adding a source hash to the cache variable name became necessary at some point during the development of the CMake staging branch, as checks for the same flags could occur with different sources in a single `cmake` invocation. Is this scenario possible in the master branch? If not, perhaps this functionality can be dropped?
2a97482f7892ee2b55955850c92ffd985a58c0a0
I'm still not convinced about the last commit, as the situation "when `working_linker_werror_flag` are changed" never occurs duri
...
(https://github.com/bitcoin/bitcoin/pull/30732#issuecomment-2353274116)
Adding a source hash to the cache variable name became necessary at some point during the development of the CMake staging branch, as checks for the same flags could occur with different sources in a single `cmake` invocation. Is this scenario possible in the master branch? If not, perhaps this functionality can be dropped?
2a97482f7892ee2b55955850c92ffd985a58c0a0
I'm still not convinced about the last commit, as the situation "when `working_linker_werror_flag` are changed" never occurs duri
...