💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1657054147)
tested ACK 69abfd3db1

all my feedback is very optional, code looks fine as is. I saw the previous review comments & update around wording suggestions & don't think it's worth much churn just for that, but left my language thoughts just for consideration.

agreed that it makes most sense to return just the totals here, and that generall the `IsTerrible` info is an internal implementation detail that would ideally not be exposed to the users.

I considered the approach of the RPC endpoint e
...

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278512304)
```suggestion
{RPCResult::Type::NUM, "new", "number of addresses in the new table, which represent potential peers."},
```

the fact that they are periodically tested seems like an implementation detail of addrman and feels a little confusing here.

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278512805)
could also check that the values for other networks are 0

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278512102)
```suggestion
{RPCResult::Type::NUM, "total", "total number of addresses in both new/tried tables"},
```

having the clause "from a network" only on total field (vs also on new and tried) is confusing. a bit redundant anyways since these fields are all nested within the networks

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278512493)
```suggestion
{RPCResult::Type::NUM, "tried", "number of addresses in the tried table, which represent peers with whom the node has successfully connected to in the past."},
```

if you take the previous suggestion, I'd update this too for consistency

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278516272)
reading the chatGPT suggestion, if you want more detail, I think it'd make more sense to use the part that says they are recently discovered but have not yet been successfully connected to.

💬 amitiuttarwar commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1657055010)
I think this PR should be in draft right now since it builds on top of #27511 & has some outdated changes / the two commits are represented as a "merge" commit here instead of separately. I would either put it into draft or keep it up-to-date in a form that would be ready for master assuming sufficient review.

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278521260)
It may be good to enumerate why this is needed (if at all).

💬 MarcoFalke commented on pull request "qa, doc: Fix comment":
(https://github.com/bitcoin/bitcoin/pull/28181#discussion_r1278522512)
```suggestion
Check that there all python files in this directory are categorized as a test script or meta script.
```

💬 YellowRoseCx commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1657075910)
Replace by fee makes double spending easier and harm's Bitcoin's ability to be used as a currency in my opinion

⚠️ MarcoFalke opened an issue: "lint: MYPY_CACHE_DIR is unused"
(https://github.com/bitcoin/bitcoin/issues/28183)
See also the discussion in https://github.com/bitcoin/bitcoin/pull/24794/files#r844848504 . However, the variable is completely unused:

```
$ git grep MYPY_CACHE_DIR
test/lint/lint-python.py:MYPY_CACHE_DIR = f"{os.getenv('BASE_ROOT_DIR', '')}/test/.mypy_cache"
```

Not sure if it was ever needed. If not, it should be removed. cc @fjahr

💬 hebasto commented on pull request "qa, doc: Fix comment":
(https://github.com/bitcoin/bitcoin/pull/28181#discussion_r1278534555)
Thanks! Updated.

📝 fjahr opened a pull request: "lint: fix custom mypy cache dir setting"
(https://github.com/bitcoin/bitcoin/pull/28184)
fixes #28183

The custom cache dir for `mypy` can only be set via an environment variable, setting the `MYPY_CACHE_DIR` variable in the program is not sufficient. This error was introduced while translating the shell script to python.

See also the mypy documentation: https://mypy.readthedocs.io/en/stable/config_file.html#confval-cache_dir

🤔 MarcoFalke reviewed a pull request: "lint: fix custom mypy cache dir setting"
(https://github.com/bitcoin/bitcoin/pull/28184#pullrequestreview-1553612167)
left some questions

💬 MarcoFalke commented on pull request "lint: fix custom mypy cache dir setting":
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1278539461)
Why is this needed?

💬 MarcoFalke commented on pull request "lint: fix custom mypy cache dir setting":
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1278539545)
This will write to `/test/` if a user is running this script:

```
# ./test/lint/lint-python.py
Success: no issues found in 269 source files
# ls /test/.mypy_cache/
3.11 CACHEDIR.TAG

📝 MarcoFalke opened a pull request: " ci: Use hard-coded root path for CI containers (bugfix)"
(https://github.com/bitcoin/bitcoin/pull/28185)
Currently the CI system will fail if the git folder that holds the Bitcoin Core source is moved from one location to another.

Fix this by just using a single hard-coded root path *inside* the CI system containers.

Steps to test:

* Fresh install of your operating system, or alternatively, clear the depends cache with: `docker volume rm ci_win64_depends`
* Run the CI system: `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh`
* Move the git folder: `p
...

💬 MarcoFalke commented on pull request "ci: Switch more tasks to self-hosted":
(https://github.com/bitcoin/bitcoin/pull/21652#issuecomment-1657098200)
Looks like there is an intermittent issue, which is fixed in podman 4.1:

```
#135258 REDUCE cov: 2818 ft: 7924 corp: 483/10261b lim: 254 exec/s: 3468 rss: 251Mb L: 15/177 MS: 1 EraseBytes-
#135494 NEWError: timed out waiting for file /var/lib/containers/storage/overlay-containers/2b5173104c7716f28471c2aed46932cd57b0904326ef3b5e3c9c0462dad553a6/userdata/75976ef6638693018268a2b4187292027a833405bd50e9ab4a3ddccae789be0b/exit/2b5173104c7716f28471c2aed46932cd57b0904326ef3b5e3c9c0462dad553a6: inte
...

💬 fanquake commented on pull request "qa, doc: Fix comment":
(https://github.com/bitcoin/bitcoin/pull/28181#discussion_r1278542958)
```suggestion
Check that all python files in this directory are categorized
```

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1657098924)
(Feel free to push the `chattr` changes here in the meantime. Obviously the CI won't pass, but people can start initial review)

💬 hebasto commented on pull request "qa, doc: Fix comment":
(https://github.com/bitcoin/bitcoin/pull/28181#discussion_r1278543285)
Done.