💬 ryanofsky commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1930671864)
> This leads to an alternative solution to not increase the memory footprint: Move the 32+32 bits used for nTx and nChainTx into a single 64-bit blob, where the 16 bit are for nTx and the remaining for nChainTx. But, again, the overall heap memory usage shouldn't change much, either way.

This does seem like an nice and easy solution. Would be good to incorporate in #29331

As a tangent, I was thinking along the same lines last week before implementing #29370 and came up with worse solution,
...

💬 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?