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

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1481099257)
We have to override below, otherwise a build where the user sets CC and CCX won't work, because there will be no target or SDK flags, and all configure checks / compilation will fail.

💬 TheCharlatan commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1481128674)
Right, what I'm suggesting is keeping the `override` lines and:
```diff
diff --git a/depends/hosts/darwin.mk b/depends/hosts/darwin.mk
index 9e201eb2bc..d31ac398a4 100644
--- a/depends/hosts/darwin.mk
+++ b/depends/hosts/darwin.mk
@@ -9,2 +9,2 @@ OSX_SDK=$(SDK_PATH)/Xcode-$(XCODE_VERSION)-$(XCODE_BUILD_ID)-extracted-SDK-with-
-CC = clang
-CXX = clang++
+darwin_CC = clang
+darwin_CXX = clang++
@@ -46,0 +47,2 @@ override CXX += $(host_and_SDK)
+override darwin_CC += $(host_and_SDK)
+o
...