📝 fanquake opened a pull request: "test: remove `GetRNGState` lsan suppression"
(https://github.com/bitcoin/bitcoin/pull/27362)
I am no-longer seeing this, testing with the native_asan job over `x86_64` (Ubuntu 22.04) and `aarch64` (Fedora 37). Can anyone recreate the false-positive?
Based on #27360 (which works locally; unclear what we are doing with Cirrus), which I need for working aarch64 builds.
(https://github.com/bitcoin/bitcoin/pull/27362)
I am no-longer seeing this, testing with the native_asan job over `x86_64` (Ubuntu 22.04) and `aarch64` (Fedora 37). Can anyone recreate the false-positive?
Based on #27360 (which works locally; unclear what we are doing with Cirrus), which I need for working aarch64 builds.
💬 dergoegge commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1152094594)
```suggestion
TRACE6(net, evicted_inbound_connection,
```
The logic in `net` exclusively evicts inbound peers and there is more eviction logic in `net_processing` for outbound connections. Maybe adding separate trace points for both makes sense?
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1152094594)
```suggestion
TRACE6(net, evicted_inbound_connection,
```
The logic in `net` exclusively evicts inbound peers and there is more eviction logic in `net_processing` for outbound connections. Maybe adding separate trace points for both makes sense?
📝 fanquake opened a pull request: "ci: use LLVM/clang-16 in native_fuzz (ASAN) job"
(https://github.com/bitcoin/bitcoin/pull/27363)
Similar to #27298.
(https://github.com/bitcoin/bitcoin/pull/27363)
Similar to #27298.
💬 fanquake commented on pull request "test: remove `GetRNGState` lsan suppression":
(https://github.com/bitcoin/bitcoin/pull/27362#issuecomment-1488816059)
Switched to basing on #27363.
(https://github.com/bitcoin/bitcoin/pull/27362#issuecomment-1488816059)
Switched to basing on #27363.
💬 fanquake commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1488816966)
native_fuzz split into #27363, as that is not blocked up upstream image issues/decisions.
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1488816966)
native_fuzz split into #27363, as that is not blocked up upstream image issues/decisions.
💬 hebasto commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152104910)
Before this change the value of `m_chainman.m_best_header` was synced with the `UpdateBlockAvailability()` call. Now they are not. Is it safe?
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152104910)
Before this change the value of `m_chainman.m_best_header` was synced with the `UpdateBlockAvailability()` call. Now they are not. Is it safe?
💬 hebasto commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152127765)
That's unfortunate that we call a logging function while holding a lock. Maybe
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2697,10 +2696,15 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
void PeerManagerImpl::UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer,
const CBlockIndex& last_header, bool received_new_header, bool may_have_more_headers)
{
- if (peer.nUnconnectingHeaders > 0) {
- LogPrint(
...
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152127765)
That's unfortunate that we call a logging function while holding a lock. Maybe
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2697,10 +2696,15 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
void PeerManagerImpl::UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer,
const CBlockIndex& last_header, bool received_new_header, bool may_have_more_headers)
{
- if (peer.nUnconnectingHeaders > 0) {
- LogPrint(
...
👍 brunoerg approved a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214)
re-ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
(https://github.com/bitcoin/bitcoin/pull/27214)
re-ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
💬 hebasto commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152158505)
nm, it will cause double lock.
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152158505)
nm, it will cause double lock.
💬 TheCharlatan commented on pull request "guix: use python-minimal (3.9)":
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1488878073)
Concept ACK
The `python-minimal` derivation is defined in https://github.com/guix-mirror/guix/blob/v1.4.0/gnu/packages/python.scm. The `python-minimal` package inherits from the `python-3`, which in turn inherits from `python-2.7`. If I am reading this correctly, the baseline `python` package requires the following input dependencies: `bzip2, expat, gdbm, libffi, sqlite, openssl, readline, zlib, tcl, tk`, while the `python-minimal` package requires `expat, libffi, openssl, zlib`. Seems like a
...
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1488878073)
Concept ACK
The `python-minimal` derivation is defined in https://github.com/guix-mirror/guix/blob/v1.4.0/gnu/packages/python.scm. The `python-minimal` package inherits from the `python-3`, which in turn inherits from `python-2.7`. If I am reading this correctly, the baseline `python` package requires the following input dependencies: `bzip2, expat, gdbm, libffi, sqlite, openssl, readline, zlib, tcl, tk`, while the `python-minimal` package requires `expat, libffi, openssl, zlib`. Seems like a
...
💬 dergoegge commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152170209)
Yes that is fine because `m_best_header` is not used by `UpdateBlockAvailability` afaict.
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1152170209)
Yes that is fine because `m_best_header` is not used by `UpdateBlockAvailability` afaict.
📝 fanquake opened a pull request: "ci: set docker run --ulimit to workaround Valgrind assertion"
(https://github.com/bitcoin/bitcoin/pull/27364)
Running the `native_fuzz_with_valgrind_job`, on aarch64 (Fedora 37), I've seen the following:
```bash
Run addr_info_deserialize with args ['valgrind', '--quiet', '--error-exitcode=1', '/home/fedora/ci_scratch/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', '/home/fedora/ci_scratch/ci/scratch/qa-assets/fuzz_seed_corpus/addr_info_deserialize']
valgrind: m_libcfile.c:66 (vgPlain_safe_fd): Assertion 'newfd >= VG_(fd_hard_limit)' failed.
valgrind: m_libcfile
...
(https://github.com/bitcoin/bitcoin/pull/27364)
Running the `native_fuzz_with_valgrind_job`, on aarch64 (Fedora 37), I've seen the following:
```bash
Run addr_info_deserialize with args ['valgrind', '--quiet', '--error-exitcode=1', '/home/fedora/ci_scratch/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', '/home/fedora/ci_scratch/ci/scratch/qa-assets/fuzz_seed_corpus/addr_info_deserialize']
valgrind: m_libcfile.c:66 (vgPlain_safe_fd): Assertion 'newfd >= VG_(fd_hard_limit)' failed.
valgrind: m_libcfile
...
💬 glozow commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1488942459)
> @glozow want to take a look at the mempool tests?
LGTM, makes sense to generate 1 to clear mempool after subtests. changing who calls `generate` shouldn't impact mempool_package_limits.
I haven't taken the time to verify that this fixes the test intermittent failure.
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1488942459)
> @glozow want to take a look at the mempool tests?
LGTM, makes sense to generate 1 to clear mempool after subtests. changing who calls `generate` shouldn't impact mempool_package_limits.
I haven't taken the time to verify that this fixes the test intermittent failure.
💬 Tracachang commented on issue "Export a watch wallet only (with descriptors and without private keys) for an air gap setup":
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1488988174)
@pinheadmz
I have followed all the steps using the bash commands from your answer and I am unable to create a receiving taproot address from the GUI (it does not show bech2m but it shows both base58 (legacy & p2sh-segwit and bech32) if I select the watch_wallet, however if I select the "original_wallet" it appears in GUI all 4 including bech32m.
In any case, from console I can `getnewaddress "test" "bech32m"` and it works but for some reason not on GUI.
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1488988174)
@pinheadmz
I have followed all the steps using the bash commands from your answer and I am unable to create a receiving taproot address from the GUI (it does not show bech2m but it shows both base58 (legacy & p2sh-segwit and bech32) if I select the watch_wallet, however if I select the "original_wallet" it appears in GUI all 4 including bech32m.
In any case, from console I can `getnewaddress "test" "bech32m"` and it works but for some reason not on GUI.
💬 TheCharlatan commented on pull request "guix: use python-minimal (3.9)":
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1489001205)
ACK d0e571ebb187
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
53aa12b7fe92d92eaf81a51a8f5d534f92a2f18f0218f29fdf3221d817a53c04 guix-build-d0e571ebb187/output/aarch64-linux-gnu/SHA256SUMS.part
01467530b06b90114561e86968710250e3648fe4aba5c228781ccf9ac72394c2 guix-build-d0e571ebb187/output/aarch64-linux-gnu/bitcoin-d0e571ebb187-aarch64-linux-gnu-debug.tar.gz
fdf6dfc901e371b4e11a7f69eb2ff9b36a3ac393255862bad1dcd45b
...
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1489001205)
ACK d0e571ebb187
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
53aa12b7fe92d92eaf81a51a8f5d534f92a2f18f0218f29fdf3221d817a53c04 guix-build-d0e571ebb187/output/aarch64-linux-gnu/SHA256SUMS.part
01467530b06b90114561e86968710250e3648fe4aba5c228781ccf9ac72394c2 guix-build-d0e571ebb187/output/aarch64-linux-gnu/bitcoin-d0e571ebb187-aarch64-linux-gnu-debug.tar.gz
fdf6dfc901e371b4e11a7f69eb2ff9b36a3ac393255862bad1dcd45b
...
💬 jonatack commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1489034354)
> I found a few other similar mechanisms in other modules, dunno if this change should apply to them as well
Circling back for completeness to cover these two questions.
> https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/validation.cpp#L2565-L2569
This code appends warning messages to the `UpdateTipLog()` logging and looks correct.
> https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/warnings.cpp#L55-L57
The `<h
...
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1489034354)
> I found a few other similar mechanisms in other modules, dunno if this change should apply to them as well
Circling back for completeness to cover these two questions.
> https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/validation.cpp#L2565-L2569
This code appends warning messages to the `UpdateTipLog()` logging and looks correct.
> https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/warnings.cpp#L55-L57
The `<h
...
👍 Ayush170-Future approved a pull request: "ci: cleanup of CI_EXEC & CI_EXEC_ROOT"
(https://github.com/bitcoin/bitcoin/pull/27333)
(https://github.com/bitcoin/bitcoin/pull/27333)
💬 furszy commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1489236967)
Before the text; this is not a blocking comment. Purely about adding some ideas.
> I'm still slightly hesitating though. One question that still bothers me is whether we should expose `blank` flag at all. It's an implementation detail and we only need it for the functional tests. It doesn't seem like there is any observable by the user behaviour of the flag, so why test it with functional tests? Unit tests are better fit for testing internal invariants.
Hmm, I also agree that exposing an i
...
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1489236967)
Before the text; this is not a blocking comment. Purely about adding some ideas.
> I'm still slightly hesitating though. One question that still bothers me is whether we should expose `blank` flag at all. It's an implementation detail and we only need it for the functional tests. It doesn't seem like there is any observable by the user behaviour of the flag, so why test it with functional tests? Unit tests are better fit for testing internal invariants.
Hmm, I also agree that exposing an i
...
💬 arnabnandikgp commented on issue "gen-manpages output depends on build options, so needs to check them":
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1489328037)
wanted to know whether this issue is still active as i am interested in this issue..
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1489328037)
wanted to know whether this issue is still active as i am interested in this issue..
💬 ProofOfKeags commented on issue "Manual-pruning cursor rewind":
(https://github.com/bitcoin/bitcoin/issues/19807#issuecomment-1489330250)
> serving old blocks to bootstrapping nodes
a node should just serve the blocks it has. The cursor rewind should cause it to download earlier blocks as if it was doing IBD only it doesn't have to do any consensus validation except that the block data matches the expected blockhash
> wallet rescans
A rescan has to go back to the wallet birthday irrespective of the pruning cursor.
One other thing to note. I no longer personally need this feature, but I'm simply describing the need as I
...
(https://github.com/bitcoin/bitcoin/issues/19807#issuecomment-1489330250)
> serving old blocks to bootstrapping nodes
a node should just serve the blocks it has. The cursor rewind should cause it to download earlier blocks as if it was doing IBD only it doesn't have to do any consensus validation except that the block data matches the expected blockhash
> wallet rescans
A rescan has to go back to the wallet birthday irrespective of the pruning cursor.
One other thing to note. I no longer personally need this feature, but I'm simply describing the need as I
...
💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1489476165)
@hebasto Thanks for the help for getting the CI/CD fixed. The fact that the version of libevent was being pinned to the earlier version explains why testing didn't catch this.
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1489476165)
@hebasto Thanks for the help for getting the CI/CD fixed. The fact that the version of libevent was being pinned to the earlier version explains why testing didn't catch this.