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

💬 maflcko commented on pull request "test: handle wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/pull/29395#issuecomment-1931492134)
Can you explain *why* this fixes the issue? What are the steps to reproduce the issue?

💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1481054678)
Could use one of the other nodes and restart?

💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1481055367)
```suggestion

tx = self.miniwallet.send_self_transfer(from_node=node)

```

It already exists

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1931513793)
Could include the test https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1927200215 , or did you want to leave that for later?

💬 TheCharlatan commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1481087970)
Why is this overriding `CC` and `CXX` here (and below)? Seems like this causes the current CI failure, or at least calling this `darwin_CC` and `darwin_CXX` allows me to properly override everything with `AR=llvm-ar-17 OBJDUMP=llvm-objdump-17 RANLIB=llvm-ranlib-17 STRIP=llvm-strip-17 CC=clang-17 CXX=clang++-17 make HOST=arm64-apple-darwin -j 30` and complete a depends build on my machine.