⚠️ MarcoFalke reopened an issue: "test: use-of-uninitialized-value in sqlite3Strlen30"
(https://github.com/bitcoin/bitcoin/issues/27222)
https://cirrus-ci.com/task/5021971277152256?logs=ci#L3656
```
wallet/test/feebumper_tests.cpp(18): Entering test suite "feebumper_tests"
wallet/test/feebumper_tests.cpp(42): Entering test case "external_max_weight_test"
2023-01-30T17:19:34Z Seed: Setting random seed for current tests to RANDOM_CTX_SEED=231d587d0169ecab6befbed75f49c95aa84567b2750479dca13bd7471f2627e2
2023-01-30T17:19:34.255341Z [test] [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v24.99.0-a55717c914f8 (rel
...
(https://github.com/bitcoin/bitcoin/issues/27222)
https://cirrus-ci.com/task/5021971277152256?logs=ci#L3656
```
wallet/test/feebumper_tests.cpp(18): Entering test suite "feebumper_tests"
wallet/test/feebumper_tests.cpp(42): Entering test case "external_max_weight_test"
2023-01-30T17:19:34Z Seed: Setting random seed for current tests to RANDOM_CTX_SEED=231d587d0169ecab6befbed75f49c95aa84567b2750479dca13bd7471f2627e2
2023-01-30T17:19:34.255341Z [test] [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v24.99.0-a55717c914f8 (rel
...
💬 MarcoFalke commented on issue "Replace all of the RecursiveMutex instances with the Mutex ones":
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514926359)
Steps to reproduce on a fresh install of Fedora 38:
* `dnf install gcc-c++ libtool make autoconf automake python3 clang llvm lbzip2 patch xz cmake curl wget htop git vim ccache libevent-devel boost-devel -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core`
* `./autogen.sh && ./configure CC=clang CXX=clang++ --with-sanitizers=thread && make -j $(nproc)`
* `TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=
...
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514926359)
Steps to reproduce on a fresh install of Fedora 38:
* `dnf install gcc-c++ libtool make autoconf automake python3 clang llvm lbzip2 patch xz cmake curl wget htop git vim ccache libevent-devel boost-devel -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core`
* `./autogen.sh && ./configure CC=clang CXX=clang++ --with-sanitizers=thread && make -j $(nproc)`
* `TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=
...
💬 MarcoFalke commented on issue "Replace all of the RecursiveMutex instances with the Mutex ones":
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514928912)
<details><summary>tsan</summary>
```
node0 stderr ==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=13857)
Cycle in lock order graph: M0 (0x7b6400005068) => M1 (0x7b440001ffa8) => M2 (0x7b5400003ee0) => M0
Mutex M1 acquired here while holding mutex M0 in thread T10:
#0 pthread_mutex_lock <null> (bitcoind+0x1a91af) (BuildId: 1db3d87bed913bc0e4da6baf5872efb9abf647be)
#1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_
...
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514928912)
<details><summary>tsan</summary>
```
node0 stderr ==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=13857)
Cycle in lock order graph: M0 (0x7b6400005068) => M1 (0x7b440001ffa8) => M2 (0x7b5400003ee0) => M0
Mutex M1 acquired here while holding mutex M0 in thread T10:
#0 pthread_mutex_lock <null> (bitcoind+0x1a91af) (BuildId: 1db3d87bed913bc0e4da6baf5872efb9abf647be)
#1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_
...
⚠️ ajtowns opened an issue: "Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/issues/27494)
### Please describe the feature you'd like to see added.
From https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1513814497:
> This brings up a related issue in that we can't have multiple signets in the same base datadir. All signets use the signet datadir, so any change of parameter may result in a bad chainstate for that signet node.
>
> I think we should instead look at implementing a more robust multiple signet solution that allows for multiple configurable consensus rules in
...
(https://github.com/bitcoin/bitcoin/issues/27494)
### Please describe the feature you'd like to see added.
From https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1513814497:
> This brings up a related issue in that we can't have multiple signets in the same base datadir. All signets use the signet datadir, so any change of parameter may result in a bad chainstate for that signet node.
>
> I think we should instead look at implementing a more robust multiple signet solution that allows for multiple configurable consensus rules in
...
💬 ajtowns commented on issue "Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/issues/27494#issuecomment-1514945137)
cc @achow101 @kallewoof
An option might be to name the chain dir `signet-XXXXXXXX` instead of `signet` where the `XX`s are the `pchMessageStart`, or perhaps to have ~40 `XX`s instead of 8 and have them be the hash160 of the signet challenge?
(https://github.com/bitcoin/bitcoin/issues/27494#issuecomment-1514945137)
cc @achow101 @kallewoof
An option might be to name the chain dir `signet-XXXXXXXX` instead of `signet` where the `XX`s are the `pchMessageStart`, or perhaps to have ~40 `XX`s instead of 8 and have them be the hash160 of the signet challenge?
💬 MarcoFalke commented on issue "Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/issues/27494#issuecomment-1514949665)
This looks like a breaking change, so one could carve out the current global default signet to keep the current folder name. Though, for other signets this breaking change seems fine.
(https://github.com/bitcoin/bitcoin/issues/27494#issuecomment-1514949665)
This looks like a breaking change, so one could carve out the current global default signet to keep the current folder name. Though, for other signets this breaking change seems fine.
💬 MarcoFalke commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171528095)
Forgot to set this for the fuzz msan task?
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171528095)
Forgot to set this for the fuzz msan task?
👍 ryanofsky approved a pull request: "move-only: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1392362907)
Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.
Confirmed move-only except for making InterpretValue function nonstatic, and moving some symbols from util to common namespace.
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1392362907)
Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.
Confirmed move-only except for making InterpretValue function nonstatic, and moving some symbols from util to common namespace.
💬 ryanofsky commented on pull request "move-only: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1171524879)
In commit "move-only: Extract common/args and common/config.cpp from util/system" (be55f545d53d44fdcf2d8ae802e9eae551d120c6)
Not important, but there is still some extra whitespace on this line
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1171524879)
In commit "move-only: Extract common/args and common/config.cpp from util/system" (be55f545d53d44fdcf2d8ae802e9eae551d120c6)
Not important, but there is still some extra whitespace on this line
💬 MarcoFalke commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171530477)
And given that the CI fails here, maybe add it to the msan task in a follow-up and only set it on the fuzz msan task?
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171530477)
And given that the CI fails here, maybe add it to the msan task in a follow-up and only set it on the fuzz msan task?
🤔 pablomartin4btc reviewed a pull request: "blockstorage: do not flush block to disk if it is already there"
(https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1392374264)
Concept ACK. (went thru notes and other related PRs mentioned on the [Review club meeting preparation webpage](https://bitcoincore.reviews/27039) - Please add label when possiblel)
(https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1392374264)
Concept ACK. (went thru notes and other related PRs mentioned on the [Review club meeting preparation webpage](https://bitcoincore.reviews/27039) - Please add label when possiblel)
💬 fanquake commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171548431)
> Forgot to set this for the fuzz msan task?
I didn't bother as that isn't run here, and wouldn't compile rn (master) in any case.
> maybe add it to the msan task in a follow-up and only set it on the fuzz msan task?
I think I'd rather defer to adding both, at the same time, in a follow up, if you're alright with that. Can drop the test commit from this PR.
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171548431)
> Forgot to set this for the fuzz msan task?
I didn't bother as that isn't run here, and wouldn't compile rn (master) in any case.
> maybe add it to the msan task in a follow-up and only set it on the fuzz msan task?
I think I'd rather defer to adding both, at the same time, in a follow up, if you're alright with that. Can drop the test commit from this PR.
🤔 darosior reviewed a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1392406641)
I really like the performance gains here, however i feel like this is making the spks management bleed into the wallet from the spkman. But there is no other way for the wallet to efficiently locate the spkman(s)..
I was looking into how to get rid of the callbacks and avoid duplicating the scriptpubkeys, and thought we might as well share a single mapping owned by `CWallet` across all spkmans. This would also have the nice property that we don't need to make sure moving forward we don't intr
...
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1392406641)
I really like the performance gains here, however i feel like this is making the spks management bleed into the wallet from the spkman. But there is no other way for the wallet to efficiently locate the spkman(s)..
I was looking into how to get rid of the callbacks and avoid duplicating the scriptpubkeys, and thought we might as well share a single mapping owned by `CWallet` across all spkmans. This would also have the nice property that we don't need to make sure moving forward we don't intr
...
💬 MarcoFalke commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#issuecomment-1514991825)
lgtm ACK 4de9c2a65f6770405f167c7392cd8371111bc4e8
(https://github.com/bitcoin/bitcoin/pull/27448#issuecomment-1514991825)
lgtm ACK 4de9c2a65f6770405f167c7392cd8371111bc4e8
🤔 jamesob reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1392348713)
Thanks to all the dedicated reviewers and my apologies for the delay. Lot of good feedback addressed.
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1392348713)
Thanks to all the dedicated reviewers and my apologies for the delay. Lot of good feedback addressed.
💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171527318)
Great catch, done.
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171527318)
Great catch, done.
💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171531164)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171531164)
Fixed.
💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171554198)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171554198)
Fixed.
💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171516957)
Done.
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171516957)
Done.
💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171514903)
Yep, that's a good point. Will do, thanks!
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171514903)
Yep, that's a good point. Will do, thanks!