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

💬 sipa commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1931178143)
@theuni That sounds to me like the only sensible approach for *any* header file: include it if you need something from it directly. If A needs B and C, and B includes C, then A should still include both B and C, because its includes shouldn't depend on the knowledge of what B depends on (as that can change from under it).

💬 Isa232323 commented on issue "Release schedule for 27.0":
(https://github.com/bitcoin/bitcoin/issues/29028#issuecomment-1931216646)
> Here is a proposed release schedule for `v27.0`, the next major release of Bitcoin Core. Dates roughly chosen as discussed in the most recent IRC meeting.
>
> ## 2024-02-01 🚧
> - Open Transifex translations for `v27.0`
> - Soft translation string freeze (no large or non-critical string changes until release)
> - Finalize and close translations for `v25.0`
>
> ## 2024-02-12 🚧 :
> - Feature freeze (bug fixes only until release)
> - Translation string freeze (no more source language ch
...

📝 hernanmarino opened a pull request: "test, assumeutxo: Add test to ensure failure when mempool not empty"
(https://github.com/bitcoin/bitcoin/pull/29394)
Add a test to ensure that loadtxoutset fials when the node's mempool is not empty, as suggested by @maflcko here: https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344713537

💬 hernanmarino commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1480923337)
I> Could add a functional test for this?

I just did, see https://github.com/bitcoin/bitcoin/pull/29394

💬 hernanmarino commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1931330307)
> There'd be [#27596 (comment)](https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344713537) if someone wants to tackle it.

Done, reviews welcome https://github.com/bitcoin/bitcoin/pull/29394

📝 araujo88 opened a pull request: "test: handle wallet_reorgrestore.py failed"
(https://github.com/bitcoin/bitcoin/pull/29395)
This PR fixes test issue mentioned in https://github.com/bitcoin/bitcoin/issues/29392.

📝 araujo88 opened a pull request: "rpc: getdescriptorinfo also returns normalized descriptor"
(https://github.com/bitcoin/bitcoin/pull/29396)
This pull request introduces enhancements to the getdescriptorinfo RPC command, specifically by incorporating the functionality to return the `normalized_descriptor` alongside the original `descriptor` information. This improvement addresses the need for a standardized format that facilitates the use of descriptors in public key operations, especially important for scenarios requiring the derivation of addresses without access to private keys.

Related issue: https://github.com/bitcoin/bitcoin
...