👍 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.

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1480787786)
In commit "test: wallet, coverage for concurrent db transactions" (cfcb9b1ecf9be5267487713fa1e112ca09a1ae55)

Would suggest calling this "handler1" instead of "handler" because otherwise when the comments below refer to a "handler" is not clear if they are referring to this variable specifically or a handler generically.

Also s/handler/batch/g throughout this function would make it more meaningful and consistent with the rest of wallet code, IMO

👍 ryanofsky approved a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1866727943)
Code review ACK cfcb9b1ecf9be5267487713fa1e112ca09a1ae55. This looks great and I think it is ready for merge. Just holding off because josibake seemed ready to review https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1930372190 and might have more feedback.

I did leave some comments but they are all minor and can be ignored. I was was also wondering about this in the PR description:

re: https://github.com/bitcoin/bitcoin/pull/29112#issue-2047565192

> To avoid this situation, we
...

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1480783903)
In commit "tests: Test for concurrent writes with db tx" (548ecd11553bea28bc79e6f9840836283e9c4e99)

Could there be a comment to explain unsafesqlitesync? Is it just to make the test faster, or is it actually important to the functioning of the test?

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1480796544)
In commit "test: wallet, coverage for concurrent db transactions" (cfcb9b1ecf9be5267487713fa1e112ca09a1ae55

Nothing about this test seems sqlite-specific. Seems like it would be nicer to call `TestDatabases` and be generic.

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1480789358)
In commit "test: wallet, coverage for concurrent db transactions" (cfcb9b1ecf9be5267487713fa1e112ca09a1ae55)

Would be good to clarify why it is not possible. Maybe "Which must not be possible because batch2->TxnBegin() was never called."

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1480799534)
In commit "tests: Test for concurrent writes with db tx" (548ecd11553bea28bc79e6f9840836283e9c4e99)

This seems fine, but you could probably increase the probability of this by calling keypoolrefill in a loop until gettxoutsetinfo returns instead of calling it once.