👍 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
...
(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?
(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.
(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."
(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.
(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).
(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
...
(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
(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
(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
(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.
(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
...
(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?
(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?
(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
(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?
(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.
(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.
(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
...
(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
...
💬 araujo88 commented on pull request "test: handle wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/pull/29395#issuecomment-1931594513)
> Can you explain _why_ this fixes the issue? What are the steps to reproduce the issue?
AFAIK, `SyncWithValidationInterfaceQueue()` is a method used to synchronize the main thread with the validation interface queue. This synchronization ensures that all pending callbacks, which might include transaction validation, block connections, and wallet notifications, are processed before the method returns.
To reproduce the issue:
1. Set up a testing environment with multiple nodes.
2. Creat
...
(https://github.com/bitcoin/bitcoin/pull/29395#issuecomment-1931594513)
> Can you explain _why_ this fixes the issue? What are the steps to reproduce the issue?
AFAIK, `SyncWithValidationInterfaceQueue()` is a method used to synchronize the main thread with the validation interface queue. This synchronization ensures that all pending callbacks, which might include transaction validation, block connections, and wallet notifications, are processed before the method returns.
To reproduce the issue:
1. Set up a testing environment with multiple nodes.
2. Creat
...