💬 epiccurious commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1480491658)
Question on your approach here - why drop the `try .. catch ... if not raised` instead of just replacing line 27?

💬 epiccurious commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1930691538)
Concept ACK 950f3602d61f51d3c42c051ed9139916f1ee23bd.

💬 brunoerg commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1480499623)
Because they are not necessary anymore. That try catch was a hack to check the logs because we didn't have a direct way to check in the logs that the reason was the port.

🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1866399622)
ACK cfcb9b1

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480562396)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"

This check implies that there is an exact solution at 2041 attempts, and we should be able to know what that solution is, so can we check for it?

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480563895)
In 3e2a97bd4bb275e2f0827a3099a64222c6460ed9 "coinselection: Track whether CG completed"

style nit: Open brace `{` should be on a new line for functions.

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480554649)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"

nit: A comment with the weight calculation like the ones in later test would be nice.

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480558134)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"

I think it would be useful to clarify that `expected_attempts` is the iteration limit.

The comment suggests that the check should be `<=` but the check looks for exactly this number of attempts. That seems fragile if there are further optimizations made that improve this.

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480564307)
In 04a043a973602bc74043f7c5767bcd55246913cb "coinselection: Add CoinGrinder algorithm"

style nit: Open brace `{` should be on a new line for functions.

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1930854540)
ACK 600fc562f815941b4a95c9f4f357dc6eed04be6c

👍 kristapsk approved a pull request: "i2p: log connection was refused due to arbitrary port"
(https://github.com/bitcoin/bitcoin/pull/29393#pullrequestreview-1866436287)
cr utACK 950f3602d61f51d3c42c051ed9139916f1ee23bd

💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1930911069)
I've pushed a branch here that does a large cleanup: https://github.com/theuni/bitcoin/commits/cleanup-config-h-headers/

The approach is to include `bitcoin-config.h` in _any_ file which uses a configure define, even if it would have been included by an earlier header. That gives us a safe starting point for further removals.

My approach was somewhat manual, but I think it's trustworthy:

Grab all possibly defined tokens: `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '
...

💬 mzumsande commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#discussion_r1480649475)
well, after actually looking what the tests does it doesn't matter either way, since we don't actually make a real connection here...

👍 ryanofsky approved a pull request: "test: Add makefile target for running unit tests"
(https://github.com/bitcoin/bitcoin/pull/29377#pullrequestreview-1866665176)
Tested ACK 5ca9b24da18e842e7a093dc44f6b222af73e92cf.

This is very convenient but the check-unit target does not seem to depend on the `test_bitcoin` executable, so it can run with a stale test binary.

I also noticed if I try to build and run the unit tests with `make -j12 -C src test/test_bitcoin check-unit` it will try to run the tests at the same time as building the test binary instead of waiting for the binary to be built.

I also wonder if this new `check-unit` test option is redund
...

👍 ryanofsky approved a pull request: "fuzz: remove unused `args` and `context` from `FuzzedWallet`"
(https://github.com/bitcoin/bitcoin/pull/29388#pullrequestreview-1866671180)
Code review ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac

🚀 ryanofsky merged a pull request: "fuzz: remove unused `args` and `context` from `FuzzedWallet`"
(https://github.com/bitcoin/bitcoin/pull/29388)

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1480738630)
Guess that you are duplicating `TryCreateDirectories()`?

You could just try to create the directory and lock it right away. E.g.
```c++
TryCreateDirectories(dir);
if (util::LockDirectory(dir, ".lock", /*probe_only=*/false) != util::LockResult::Success) {
// error out
}
etc..
```

Shouldn't need to execute the `fs::exist` call.

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1480729338)
What is the purpose of this extra levels?
This can just be `root_dir / G_TEST_GET_FULL_NAME()`.

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1480740777)
If this doesn't delete the .lock file, then the `fs::create_directories` call isn't needed?

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1931061991)
> Needs rebase

Yes, thanks for recognizing the CI failures.

Rebased 603a92c9881ed78e29f15c48121ce8bf35d30837 -> 3f0b8607f549b27e9bb0cc8b189252a5cd954a36 ([`pr/nofake.4`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.4) -> [`pr/nofake.5`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nofake.4-rebase..pr/nofake.5)) with test fixes needed after silent conflict with #29354