💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686444220)
I think it would help a bit, but I'm more concerned about the setter name, when in fact it's a [complex method, changing internal state](https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686312388), it's why I recommended something like `FromHexUnchecked` (the naming suggestion also applies to the non-deprected method calling this, of course).

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686444687)
posted to https://github.com/bitcoin/bitcoin/pull/30436, please resolve it here

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#issuecomment-2242801922)
`lint` is green now, sorry for the force pushes, but this can now be reviewed and tested.

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686446390)
👍, I follow-up PR makes sense here

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters once per tip change":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2242819514)
> * Introduce a new `m_tx_download_mutex` which guards the transaction download data structures including `m_txrequest`, the rolling bloom filters, and `m_orphanage`.
>
> * `m_orphanage` doesn't need its own lock anymore

In the master branch, the design of the `TxOrphanage` class is thread-safe for all purposes. I don't think that the fact that `m_orphanage` does not require additional synchronization justifies the change of `TxOrphanage` class design. The new `m_tx_download_
...

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242837403)
> Also, you can adjust the pull request description with a motivation and fix. Otherwise, it is just two random links, and reviewers will have a hard time seeing the motivation and gist of the changes.

Done. Thank you for you patience @maflcko.

👍 paplorinc approved a pull request: "lint: Use consistent out-of-tree build for python and test_runner"
(https://github.com/bitcoin/bitcoin/pull/30499#pullrequestreview-2191431156)
utACK fa8d73e86e1c11cdfe8154ab84edc1948283454b

💬 paplorinc commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686468669)
can `popd` fail if `pushd` succeeded?

💬 paplorinc commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686464727)
Now that we're setting this explicitly, does the prefixing of `CI_RETRY_EXE` still make sense in every case?
If it does, would it make sense to use it for e.g. `${CI_RETRY_EXE} curl` (like in https://github.com/bitcoin/bitcoin/blob/master/ci/test/01_base_install.sh#L88) or `${CI_RETRY_EXE} cargo build` commands as well?

💬 paplorinc commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686470495)
Unrelated, but shouldn't git clone contain `--depth=1` throughout this file?

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686476487)
Would fully agree if we were introducing a new test function, but as it is in the middle of existing `BOOST_CHECK`s, I think it's fine. Won't sweat it if I don't show up in the `git blame` after the `BOOST_CHECK_EQUAL`-refactor. And I'm happy to review such a follow-up PR.

(Original change here was suggested in https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675658574)

💬 Sjors commented on pull request "Fix lint-spelling warnings":
(https://github.com/bitcoin/bitcoin/pull/30500#issuecomment-2242862369)
ACK bccfca0382bbf00092db6e7828fc79b6ce399c5d

The new lint failure is unrelated, see #30496. Once fixed, you should rebase this to make sure.

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686485506)
> `ci/test`

`ci/lint` is completely separate from `ci/test` (well after the first commit), so there is no interaction.

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686487708)
I don't known. I had to change this due to a lint error:

```
In ci/lint/04_install.sh line 68:
popd
^--^ SC2164 (warning): Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
Did you mean:
popd || exit
For more information:
https://www.shellcheck.net/wiki/SC2164 -- Use 'popd ... || exit' or 'popd ....
^---- ⚠️ Failure generated from lint-shell.py

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686489990)
I may do this in a follow-up

💬 Sjors commented on pull request "fuzz: reduce keypool size in `scriptpubkeyman` target":
(https://github.com/bitcoin/bitcoin/pull/30494#issuecomment-2242870330)
Concept ACK

💬 paplorinc commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686491315)
k, thanks

💬 paplorinc commented on pull request "Fix lint-spelling warnings":
(https://github.com/bitcoin/bitcoin/pull/30500#issuecomment-2242875432)
Thanks for the review @Sjors.

> The new lint failure is unrelated, see https://github.com/bitcoin/bitcoin/issues/30496

Yes, it was the [PR](https://github.com/bitcoin/bitcoin/pull/30499) that triggered this change

💬 josibake commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#issuecomment-2242876699)
utACK https://github.com/bitcoin/bitcoin/commit/fa8d73e86e1c11cdfe8154ab84edc1948283454b

ran `DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter`, more as a sanity check since this isn't really testing the underlying issue this PR is fixing.

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686494462)
See https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686054773

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686494537)
Not sure about `const`ing `string_view` parameters, could only find one pre-existing case of that pattern (`GetBuriedDeployment()`). Can sympathize with forcing introduction of new names over mutation. I like how you named the first digit `hi` (maybe `high` is more precise).

Ultimately, my modification of the function is meant to be a straightforward transformation of the original code. Since it has received a few ACKs already, I am going to put off changing it for now. But might draw some in
...