💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2230695414)
nit in dbf8dbf4b1792686be70050c3b19e477769d142b: Why pass an empty array?

💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2230684594)
nit in f5f643a7cd5ab251d2491661318553fd7463acaf: Forgot to remove the function in the cpp file?

💬 stickies-v commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3117179644)
re-ACK face8123fdc10549676c6679ee3225c178a7f30c

💬 fanquake commented on pull request "p2p: rename GetAddresses -> GetAddressesUnsafe":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3117182153)
cc @mzumsande

💬 marcofleon commented on pull request "[29.x] final changes for v29.1rc1":
(https://github.com/bitcoin/bitcoin/pull/33056#issuecomment-3117319768)
nice, ACK 4c2d285b706058ece7092d03f0e5ad1112c2097f

💬 fanquake commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3117350070)
Reproduced locally (and consistently) on aarch64 and x86_64. Wondered why this isn't happening in the TSAN CI, it's because of `DEBUG_LOCKORDER`. If you drop that, then the CI will reproduce:

<details><summary>more details</summary>

```bash
134/143 Test #107: transaction_tests .................... Passed 19.46 sec
135/143 Test #116: txvalidationcache_tests .............. Passed 21.00 sec
136/143 Test #140: wallet_tests ......................... Passed 18.79 sec
137/143 Test
...

💬 fanquake commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3117355305)
ACK face8123fdc10549676c6679ee3225c178a7f30c

🚀 fanquake merged a pull request: "log: [refactor] Use info level for init logs"
(https://github.com/bitcoin/bitcoin/pull/32967)

💬 maflcko commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3117377451)
@olegrok If you want, you can test https://github.com/bitcoin/bitcoin/pull/33002

💬 Sjors commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3117486035)
Original discussion: https://github.com/bitcoin/bitcoin/pull/26716#issuecomment-1356314359

> I reckon the lower memory usage is due to using clang instead of gcc.

`pyenv install` also honors `CC=clang` (tested by setting `CC=fkfkkf` which fails)

> it'd be less lines of code.

It removes a few lines in the installers, but adds some for `pyenv global` and `pyenv init`.

Using PyEnv directly does make it easier to use multiple Python versions, in case any of our linters would benefit f
...

📝 HowHsu opened a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062)
In PackageTRUCChecks(), we calculate the in-package-parents for an in package tx, which results in the total time complexisity being O(n^2), n is the number of Txs in the package. Let's precompute the overall relationships between all transactions to make it be O(n).

💬 Sjors commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3117511064)
(added rationale to the first commit message)

🤔 janb84 reviewed a pull request: "refactor: GenTxid type safety followups"
(https://github.com/bitcoin/bitcoin/pull/33005#pullrequestreview-3055150696)
concept ACK 8e19a4375e1db6e59cdcb38e0cc3e1dab79e88ec

This PR is a followup of #32631 which addresses some of the NITS / comments that were placed in that PR. Additionally the PR simplifies `m_tx_inventory_to_send` a bit to make it `std::set<Wtxid>` based in stead of `std::set<GenTxid>` imho a good change.

- code reviewed ✅
- build and tested ✅

💬 maflcko commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#issuecomment-3117590501)
Also, it would be good to have evidence that the underlying bug is related to the close at all.

Even with code added to throw on a failed close, the error keeps happening for me:


diff:


```diff
diff --git a/ci/test/00_setup_env_i686_multiprocess.sh b/ci/test/00_setup_env_i686_multiprocess.sh
index e19a45d..4a7c3ea 100755
--- a/ci/test/00_setup_env_i686_multiprocess.sh
+++ b/ci/test/00_setup_env_i686_multiprocess.sh
@@ -16,6 +16,8 @@ export GOAL="install"
export TEST_RUNNER_EXT
...

💬 rkrux commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2230972628)
In 17bf90179097f0865500ff88b5f6d307127a8917 "tests: Check that the last hardened cache upgrade occurs"

I reluctantly find myself suggesting this because I believe it would be easier to have only one such implementation in the `test_framework.py` (similar to `skip_if_no_py_sqlite3`) for this instead of quickly duplicating in couple test files. Fine with any variant of this function that seems more suitable.

```python
def import_py_sqlite3():
try:
import sqlite3
return True
except
...

💬 yancyribbens commented on pull request "Slay BnB Mutants":
(https://github.com/bitcoin/bitcoin/pull/33060#issuecomment-3117671691)
Concept ACK. I recently found some of these by accident working on max-weight related projects.

💬 m3dwards commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2231013930)
Ideally CI will run without a patch in as many places as possible.

By checking the repository name it will run for: pushes/merges to main repo, PRs from forks and pushes to forks (where it would run on a github runner).

The `USE_CIRRUS_RUNNERS` enabled a fork to also run on Cirrus if they have bought some runners but would NOT work for a PR from a third fork to that fork. I think this is confusing and so I agree to remove this var. Forks would have to patch if they want to run on cirrus ru
...

💬 rkrux commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#issuecomment-3117737469)
I deleted my previous updated suggestion regarding importing `pysqlite3` because it is not considerably better than the current implementation.

💬 glozow commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2231065911)
I think most of the benefits of precomputing this would be realized in the loop a few lines down looking for a package_tx sibling. As is, I'm not sure how much faster precomputation is: n=2 in normal cases, and we don't need to continue computing parents once we've realized it's oversize.

💬 glozow commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2231070636)
Would you consider using DepGraph instead of a matrix of indices (it's basically the same thing but more compact)?