🚀 fanquake merged a pull request: "ci: explicitly install libclang-rt-dev in valgrind jobs"
(https://github.com/bitcoin/bitcoin/pull/27459)
(https://github.com/bitcoin/bitcoin/pull/27459)
📝 MarcoFalke opened a pull request: "rpc: Add importmempool RPC "
(https://github.com/bitcoin/bitcoin/pull/27460)
Currently it is possible to import a mempool by placing it in the datadir and starting the node. However this has many issues:
* Users aren't expected to fiddle with the datadir, possibly corrupting it
* An existing mempool file in the datadir may be overwritten
* The node needs to be restarted
* Importing an untrusted file this way is dangerous, because it can corrupt the mempool
Fix all issues by adding a new RPC.
(https://github.com/bitcoin/bitcoin/pull/27460)
Currently it is possible to import a mempool by placing it in the datadir and starting the node. However this has many issues:
* Users aren't expected to fiddle with the datadir, possibly corrupting it
* An existing mempool file in the datadir may be overwritten
* The node needs to be restarted
* Importing an untrusted file this way is dangerous, because it can corrupt the mempool
Fix all issues by adding a new RPC.
💬 glozow commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1506796551)
> > Adding a loadmempool RPC seems to be exactly what we want here?
>
> concept ACK on this, if that's the only use-case. We should probably offer a similar service over RPC first, in addition to information gathering that it's not actually being used in meaningful amounts in production. I've never come across usage myself in my production work.
#27460
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1506796551)
> > Adding a loadmempool RPC seems to be exactly what we want here?
>
> concept ACK on this, if that's the only use-case. We should probably offer a similar service over RPC first, in addition to information gathering that it's not actually being used in meaningful amounts in production. I've never come across usage myself in my production work.
#27460
💬 Sjors commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1506861773)
TIL you can quite easily verify from the clipboard (at least on macOS, probably on Linux too). No need to put things in a file. `gpg --verify` will show a prompt. Paste everything from `-----BEGIN` to the end, hit enter and then `ctrl + d`.
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1506861773)
TIL you can quite easily verify from the clipboard (at least on macOS, probably on Linux too). No need to put things in a file. `gpg --verify` will show a prompt. Paste everything from `-----BEGIN` to the end, hit enter and then `ctrl + d`.
💬 Sjors commented on issue "Write instructions on offline signing.":
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1506868821)
I think that with PSBT there's no need to go out of our way to document the legacy `signrawtransaction`.
The external signer docs kind of assume a connected device / service, so not really "offline". The PSBT doc could probably be update to use descriptor wallets rather than `createmultisig`, though creating descriptors by hand is a bit tedious at the moment.
The [multisig tutorial](https://github.com/bitcoin/bitcoin/blob/master/doc/multisig-tutorial.md) is more up to date, uses descriptor
...
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1506868821)
I think that with PSBT there's no need to go out of our way to document the legacy `signrawtransaction`.
The external signer docs kind of assume a connected device / service, so not really "offline". The PSBT doc could probably be update to use descriptor wallets rather than `createmultisig`, though creating descriptors by hand is a bit tedious at the moment.
The [multisig tutorial](https://github.com/bitcoin/bitcoin/blob/master/doc/multisig-tutorial.md) is more up to date, uses descriptor
...
👍 furszy approved a pull request: "Register `wallet::AddressPurpose` type"
(https://github.com/bitcoin-core/gui/pull/726#pullrequestreview-1383357368)
Tested ACK a45b54406dbce4fbf8a316a0e91615eb480da653
(https://github.com/bitcoin-core/gui/pull/726#pullrequestreview-1383357368)
Tested ACK a45b54406dbce4fbf8a316a0e91615eb480da653
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165491936)
For reference, this fails with:
```
test/sighash_tests.cpp(163): Leaving test case "sighash_from_data"; testing time: 3749686us
test/sighash_tests.cpp(120): Entering test case "sighash_test"
==21825== Source and destination overlap in memcpy(0x8704270, 0x8704270, 36)
==21825== at 0x488CFA0: __GI_memcpy (in /usr/libexec/valgrind/vgpreload_memcheck-arm64-linux.so)
==21825== by 0x895673: CTxIn::operator=(CTxIn const&) (transaction.h:74)
==21825== by 0x8DD22F: SignatureHashOld(CScr
...
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165491936)
For reference, this fails with:
```
test/sighash_tests.cpp(163): Leaving test case "sighash_from_data"; testing time: 3749686us
test/sighash_tests.cpp(120): Entering test case "sighash_test"
==21825== Source and destination overlap in memcpy(0x8704270, 0x8704270, 36)
==21825== at 0x488CFA0: __GI_memcpy (in /usr/libexec/valgrind/vgpreload_memcheck-arm64-linux.so)
==21825== by 0x895673: CTxIn::operator=(CTxIn const&) (transaction.h:74)
==21825== by 0x8DD22F: SignatureHashOld(CScr
...
💬 pinheadmz commented on issue "Write instructions on offline signing.":
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1506936881)
@Sjors maybe something like this in a new doc with create/sign tx commands added?
https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1487425971
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1506936881)
@Sjors maybe something like this in a new doc with create/sign tx commands added?
https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1487425971
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165506294)
Yea this, and the other failure in the benches you'll see, if you suppress this one (see below), were already known, and the whole reason I wanted to move to 3.20, rather than continuing to fight with broken tools. I'll open the build from source PR.
```bash
Running bench/bench_bitcoin (one iteration sanity check, only high priority)...
bench/bench_bitcoin -sanity-check -priority-level=high
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
...
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165506294)
Yea this, and the other failure in the benches you'll see, if you suppress this one (see below), were already known, and the whole reason I wanted to move to 3.20, rather than continuing to fight with broken tools. I'll open the build from source PR.
```bash
Running bench/bench_bitcoin (one iteration sanity check, only high priority)...
bench/bench_bitcoin -sanity-check -priority-level=high
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
...
💬 achow101 commented on pull request "Register `wallet::AddressPurpose` type":
(https://github.com/bitcoin-core/gui/pull/726#issuecomment-1506961246)
ACK a45b54406dbce4fbf8a316a0e91615eb480da653
(https://github.com/bitcoin-core/gui/pull/726#issuecomment-1506961246)
ACK a45b54406dbce4fbf8a316a0e91615eb480da653
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165532774)
Looks like gcc works fine, see also https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1163983711. Though, this won't help with the fuzz task.
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165532774)
Looks like gcc works fine, see also https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1163983711. Though, this won't help with the fuzz task.
✅ hebasto closed an issue: "Crash using getnewaddress in the console"
(https://github.com/bitcoin-core/gui/issues/725)
(https://github.com/bitcoin-core/gui/issues/725)
🚀 hebasto merged a pull request: "Register `wallet::AddressPurpose` type"
(https://github.com/bitcoin-core/gui/pull/726)
(https://github.com/bitcoin-core/gui/pull/726)
💬 ajtowns commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1507067647)
> Right, the changes for package relay only (i.e. no v3, no package RBF) would be this, persist packages across restart, and allow any ancestor package instead of just child-with-parents.
Sorry, I more just mean "what's the minimal set of additional commits that need this that results in something that's useful on its own?"
Maybe the current overarching "useful thing" is just "package mempool acceptance works in regtest via rpc in a way that would be acceptable to expose on mainnet for pac
...
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1507067647)
> Right, the changes for package relay only (i.e. no v3, no package RBF) would be this, persist packages across restart, and allow any ancestor package instead of just child-with-parents.
Sorry, I more just mean "what's the minimal set of additional commits that need this that results in something that's useful on its own?"
Maybe the current overarching "useful thing" is just "package mempool acceptance works in regtest via rpc in a way that would be acceptable to expose on mainnet for pac
...
💬 MarcoFalke commented on pull request "ci: set docker run --ulimit to workaround Valgrind assertion":
(https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1507077524)
I tried to reproduce on fedora 37 on current master, and it passed with podman
(https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1507077524)
I tried to reproduce on fedora 37 on current master, and it passed with podman
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165620563)
The fuzz task seems to be passing, see https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1507077524, so no need to bump valgrind. You can simply use gcc:
```diff
diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index 97b85755e..794a2dcca 100755
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -8,10 +8,10 @@ export LC_ALL=C.UTF-8
export CI_IMAGE_NAME_TAG="debian:bookworm"
export CON
...
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165620563)
The fuzz task seems to be passing, see https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1507077524, so no need to bump valgrind. You can simply use gcc:
```diff
diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index 97b85755e..794a2dcca 100755
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -8,10 +8,10 @@ export LC_ALL=C.UTF-8
export CI_IMAGE_NAME_TAG="debian:bookworm"
export CON
...
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165623156)
I'll still open a PR for discussion, as I don't think our reponse to a tool being broken/having known issues, should be, change compiler, as opposed to just using a non-broken version of the tool?
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165623156)
I'll still open a PR for discussion, as I don't think our reponse to a tool being broken/having known issues, should be, change compiler, as opposed to just using a non-broken version of the tool?
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165625114)
I guess we'd also have to document that (some versions of?) Valgrind can't be used under Clang?
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165625114)
I guess we'd also have to document that (some versions of?) Valgrind can't be used under Clang?
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165632857)
> I guess we'd also have to document that (some versions of?) Valgrind can't be used under Clang?
Sure if you think it is useful, but that seems unrelated to the CI system.
> I'll still open a PR for discussion
I am not a fan of starting to build our own package manager, unless it is for a temporary workaround. And if something is a temporary workaround, one might as well pick the one that is the least effort to implement/revert/test/maintain.
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165632857)
> I guess we'd also have to document that (some versions of?) Valgrind can't be used under Clang?
Sure if you think it is useful, but that seems unrelated to the CI system.
> I'll still open a PR for discussion
I am not a fan of starting to build our own package manager, unless it is for a temporary workaround. And if something is a temporary workaround, one might as well pick the one that is the least effort to implement/revert/test/maintain.
💬 MarcoFalke commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507099435)
> > Reproducible
>
> 100%. Running `time FILE_ENV="./ci/test/00_setup_env_native_tsan.sh" ./ci/test_run_all.sh` on a aarch64 machine running Fedora 37.
Idk. Locally this fails for me when building bdb:
```
Extracting bdb...
/root/bitcoin-core/depends/sources/db-4.8.30.NC.tar.gz: OK
Preprocessing bdb...
Configuring bdb...
patching file dbinc/atomic.h
patching file mp/mp_fget.c
patching file mp/mp_mvcc.c
patching file mp/mp_region.c
patching file mutex/mut_method.c
patching file
...
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507099435)
> > Reproducible
>
> 100%. Running `time FILE_ENV="./ci/test/00_setup_env_native_tsan.sh" ./ci/test_run_all.sh` on a aarch64 machine running Fedora 37.
Idk. Locally this fails for me when building bdb:
```
Extracting bdb...
/root/bitcoin-core/depends/sources/db-4.8.30.NC.tar.gz: OK
Preprocessing bdb...
Configuring bdb...
patching file dbinc/atomic.h
patching file mp/mp_fget.c
patching file mp/mp_mvcc.c
patching file mp/mp_region.c
patching file mutex/mut_method.c
patching file
...