💬 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

💬 ThunderCat1000 commented on issue "Monkey Test Feature (MTF)":
(https://github.com/bitcoin/bitcoin/issues/29389#issuecomment-1931072095)
It's hard to understand, but it's obvious in hindsight. The problem is that if I go to send a test transaction, and one bug is in the amount field, and another is in the recipient field, you could lose all funds for simply trying to test a dust transaction. Wouldn't it be nice if this were mitigated with a test feature in the wallet interface? The idea is that you could test the recipient field with a predefined amount so that these two bugs don't compound, then you can proceed to use the amount
...

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

💬 epiccurious commented on pull request "test: fix intermittent failure in `rpc_setban.py --v2transport`, run it in CI":
(https://github.com/bitcoin/bitcoin/pull/29372#issuecomment-1931126349)
Concept ACK cc87ee4c3934028e78a59de509951ff7226ec80d.

💬 furszy commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1480802224)
Same as before, would be better to not use `assert_greater_than`, the wallet is receiving exactly 17 btc.

💬 furszy commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1480800805)
In e73a56f13:

This change should be in the first commit, not here. And the sync call should be done prior to calling `getbalances()` too (one line above the current one).

💬 furszy commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1480800235)
nit: could write it:

```c++
const TxSize tx_size{CalculateMaximumSignedTxSize(CTransaction(rawTx), pwallet.get())};
const CAmount fee_from_size{fee_rate.GetFee(tx_size.vsize)};
const std::optional<CAmount> total_bump_fees{pwallet->chain().calculateCombinedBumpFee(outpoints_spent, fee_rate)};
CAmount effective_value = total_input_value - fee_from_size - total_bump_fees.value_or(0);
```

💬 furszy commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1480797443)
Also, would be better to check for the exact amount. You are sending exactly 17 btc to the wallet.