💬 dergoegge commented on pull request "build: pass sanitize flags to instrument libsecp256k1 code":
(https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1611472816)
Concept ACK
All of the CI task that make use of the `integer` sanitizer are probably gonna fail.
(https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1611472816)
Concept ACK
All of the CI task that make use of the `integer` sanitizer are probably gonna fail.
💬 furszy commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1611484179)
> What happened to [fa23292](https://github.com/bitcoin/bitcoin/commit/fa2329285c918eb7a627f31df2a493b967a9d660)?
Vanished after #24914 merge :).
We no longer iterate over all the database records without any ordering. Now the wallet loads each specific group of records separately (e.g. legacy records, descriptors records, txs, etc).
The reason behind fa232928 was that the upper level method `LoadWallet` was only treating few specific `ReadKeyValue` errors as corruption ones (we were ad
...
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1611484179)
> What happened to [fa23292](https://github.com/bitcoin/bitcoin/commit/fa2329285c918eb7a627f31df2a493b967a9d660)?
Vanished after #24914 merge :).
We no longer iterate over all the database records without any ordering. Now the wallet loads each specific group of records separately (e.g. legacy records, descriptors records, txs, etc).
The reason behind fa232928 was that the upper level method `LoadWallet` was only treating few specific `ReadKeyValue` errors as corruption ones (we were ad
...
🤔 vasild reviewed a pull request: "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py"
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1502905052)
Approach ACK b5e0398d8ced2ee998756e67fb90bc8d604d4ec3
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1502905052)
Approach ACK b5e0398d8ced2ee998756e67fb90bc8d604d4ec3
💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245093887)
Can this use `self.ADDRV2_NET_NAME`?
```suggestion
assert self.net in self.ADDRV2_NET_NAME
```
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245093887)
Can this use `self.ADDRV2_NET_NAME`?
```suggestion
assert self.net in self.ADDRV2_NET_NAME
```
💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245108107)
When would this socket be closed if `keep_alive` is true?
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245108107)
When would this socket be closed if `keep_alive` is true?
💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245183148)
The port is serialized last, but in the comment is comes after `networkID`. Would it be better to calculate the message size separately, given an array of addresses as input?
<details>
<summary>[patch] calculate addrv2 message size separately</summary>
```diff
diff --git i/test/functional/p2p_addrv2_relay.py w/test/functional/p2p_addrv2_relay.py
index 23d7c94fd0..f9a8c44be2 100755
--- i/test/functional/p2p_addrv2_relay.py
+++ w/test/functional/p2p_addrv2_relay.py
@@ -20,32 +20,27 @@
...
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245183148)
The port is serialized last, but in the comment is comes after `networkID`. Would it be better to calculate the message size separately, given an array of addresses as input?
<details>
<summary>[patch] calculate addrv2 message size separately</summary>
```diff
diff --git i/test/functional/p2p_addrv2_relay.py w/test/functional/p2p_addrv2_relay.py
index 23d7c94fd0..f9a8c44be2 100755
--- i/test/functional/p2p_addrv2_relay.py
+++ w/test/functional/p2p_addrv2_relay.py
@@ -20,32 +20,27 @@
...
💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245278854)
Does this suffice to ensure 1 block-only was opened:
<details>
<summary>[patch] check log at shutdown</summary>
```diff
diff --git i/test/functional/feature_anchors.py w/test/functional/feature_anchors.py
index e7cd642484..327533a340 100755
--- i/test/functional/feature_anchors.py
+++ w/test/functional/feature_anchors.py
@@ -91,29 +91,23 @@ class AnchorsTest(BitcoinTestFramework):
self.log.info("Ensure addrv2 support")
# Use proxies to catch outbound connections t
...
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245278854)
Does this suffice to ensure 1 block-only was opened:
<details>
<summary>[patch] check log at shutdown</summary>
```diff
diff --git i/test/functional/feature_anchors.py w/test/functional/feature_anchors.py
index e7cd642484..327533a340 100755
--- i/test/functional/feature_anchors.py
+++ w/test/functional/feature_anchors.py
@@ -91,29 +91,23 @@ class AnchorsTest(BitcoinTestFramework):
self.log.info("Ensure addrv2 support")
# Use proxies to catch outbound connections t
...
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1245286239)
That makes sense, I'll do that using a [scripted diff.](https://github.com/bitcoin/bitcoin/blob/d6ee03507f39223889a5f039c4db7204ddfb91d5/doc/developer-notes.md#scripted-diffs)
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1245286239)
That makes sense, I'll do that using a [scripted diff.](https://github.com/bitcoin/bitcoin/blob/d6ee03507f39223889a5f039c4db7204ddfb91d5/doc/developer-notes.md#scripted-diffs)
👍 ryanofsky approved a pull request: "kernel: Rm ShutdownRequested and AbortNode from validation code."
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1503219053)
Code review ACK 6eb33bd0c21b3e075fbab596351cacafdc947472. No changes since last review other than rebase.
If I can make one more suggestion about PR the description, I would suggest to make "Replace all AbortNode calls..." the first and main paragraph, and put everything else that's there below it separated by a horizontal line. I think the other text which is abstract and relies on explanations in linked PRs is going to be intimidating and not to make much sense to people who aren't followin
...
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1503219053)
Code review ACK 6eb33bd0c21b3e075fbab596351cacafdc947472. No changes since last review other than rebase.
If I can make one more suggestion about PR the description, I would suggest to make "Replace all AbortNode calls..." the first and main paragraph, and put everything else that's there below it separated by a horizontal line. I think the other text which is abstract and relies on explanations in linked PRs is going to be intimidating and not to make much sense to people who aren't followin
...
💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245290439)
oh, right, I misunderstood this, can be resolved.
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245290439)
oh, right, I misunderstood this, can be resolved.
💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611504716)
`9c46f3ba19...c1a169082a`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245084467
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611504716)
`9c46f3ba19...c1a169082a`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245084467
💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245300595)
Done.
I only changed the first `p` to `info` because `p = [p for p ...` seemed too many `p`s in one place.
:exploding_head:
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245300595)
Done.
I only changed the first `p` to `info` because `p = [p for p ...` seemed too many `p`s in one place.
:exploding_head:
💬 vasild commented on issue "Finding peers to connect to after -onlynet changes may be problematic":
(https://github.com/bitcoin/bitcoin/issues/26035#issuecomment-1611534437)
@CocolinoFan what you describe is problem 2 from the OP. A fix for it is at https://github.com/bitcoin/bitcoin/pull/27213. It would be nice if you can test and/or review that.
(https://github.com/bitcoin/bitcoin/issues/26035#issuecomment-1611534437)
@CocolinoFan what you describe is problem 2 from the OP. A fix for it is at https://github.com/bitcoin/bitcoin/pull/27213. It would be nice if you can test and/or review that.
💬 MarcoFalke commented on issue "libsecp256k1 not instrumented when building with sanitizers":
(https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1611549924)
Is this needed for anything? I guess it is odd that msan didn't complain about this, but other than that there shouldn't be anything needed here.
(https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1611549924)
Is this needed for anything? I guess it is odd that msan didn't complain about this, but other than that there shouldn't be anything needed here.
💬 dergoegge commented on issue "libsecp256k1 not instrumented when building with sanitizers":
(https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1611573124)
> Is this needed for anything? I guess it is odd that msan didn't complain about this, but other than that there shouldn't be anything needed here.
Our MSan job actually passes the correct `CFLAGS` otherwise it would (i assume) not work properly. Our oss-fuzz builds are also not affected since they also set their own `CFLAGS` and don't use `--with-sanitizers`.
I think it would be useful to do this for our fuzz targets that call secp code (e.g. the targets added in #27479), otherwise we end
...
(https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1611573124)
> Is this needed for anything? I guess it is odd that msan didn't complain about this, but other than that there shouldn't be anything needed here.
Our MSan job actually passes the correct `CFLAGS` otherwise it would (i assume) not work properly. Our oss-fuzz builds are also not affected since they also set their own `CFLAGS` and don't use `--with-sanitizers`.
I think it would be useful to do this for our fuzz targets that call secp code (e.g. the targets added in #27479), otherwise we end
...
📝 fanquake opened a pull request: "ci: build Valgrind (3.21) from source"
(https://github.com/bitcoin/bitcoin/pull/27992)
I've been using this branch for some time, for working Valgrind CI jobs on aarch64. Benefits include:
* Valgrind CI jobs work across x86_64 & aarch64.
* Can use latest (hopefully less buggy) Valgrind, rather than whatever the distro happens to package.
* No need to "bless" a specific compiler, (current discussion includes switching from Clang to GCC as a workaround).
* Valgrind from source runs significantly faster compared to the system package. i.e, when fuzzing under valgrind:
Master:
...
(https://github.com/bitcoin/bitcoin/pull/27992)
I've been using this branch for some time, for working Valgrind CI jobs on aarch64. Benefits include:
* Valgrind CI jobs work across x86_64 & aarch64.
* Can use latest (hopefully less buggy) Valgrind, rather than whatever the distro happens to package.
* No need to "bless" a specific compiler, (current discussion includes switching from Clang to GCC as a workaround).
* Valgrind from source runs significantly faster compared to the system package. i.e, when fuzzing under valgrind:
Master:
...
💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245343374)
ok gotcha about the chain interface.
I thought about it because of the goal of removing the node internal types from the subdirectories. If we stick to that path, the code move into the `src/index/init.cpp` could be seen as a regression.
The good point about moving this code somewhere else is that it could be unit tested without having to add `init.h` and `NodeContext` dependencies.
At least for now, I'm a bit inclined for the `src/node/` option but not feeling strong over it neither. I
...
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245343374)
ok gotcha about the chain interface.
I thought about it because of the goal of removing the node internal types from the subdirectories. If we stick to that path, the code move into the `src/index/init.cpp` could be seen as a regression.
The good point about moving this code somewhere else is that it could be unit tested without having to add `init.h` and `NodeContext` dependencies.
At least for now, I'm a bit inclined for the `src/node/` option but not feeling strong over it neither. I
...
💬 mzumsande commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#issuecomment-1611579553)
> Could rebase for CI?
done
(https://github.com/bitcoin/bitcoin/pull/27823#issuecomment-1611579553)
> Could rebase for CI?
done
💬 fanquake commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1611579793)
> Ah, sorry that's a bug. Can you check log_path=${BASE_SCRATCH_DIR}/sanitizer-output/tsan?
```bash
# cat tsan.25230
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=25230)
Cycle in lock order graph: M0 (0xffff75666608) => M1 (0xffff7566a868) => M2 (0xffff7566aa18) => M0
Mutex M1 acquired here while holding mutex M0 in main thread:
#0 pthread_rwlock_wrlock <null> (test_bitcoin+0x1537a0) (BuildId: 501b437270597fa9a7a48cb4f66b32eca15a18
...
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1611579793)
> Ah, sorry that's a bug. Can you check log_path=${BASE_SCRATCH_DIR}/sanitizer-output/tsan?
```bash
# cat tsan.25230
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=25230)
Cycle in lock order graph: M0 (0xffff75666608) => M1 (0xffff7566a868) => M2 (0xffff7566aa18) => M0
Mutex M1 acquired here while holding mutex M0 in main thread:
#0 pthread_rwlock_wrlock <null> (test_bitcoin+0x1537a0) (BuildId: 501b437270597fa9a7a48cb4f66b32eca15a18
...
💬 TheCharlatan commented on pull request "refactor: Move sock from util to common":
(https://github.com/bitcoin/bitcoin/pull/27989#issuecomment-1611583183)
What about all the other networking and wallet related files in `/util`? E.g. we should probably also move `asmap.cpp`, `bip32.cpp`, `bytevectorhash.cpp`, `error.cpp`, `fees.cpp`, `syserror.cpp`, `message.cpp`, and `readwritefile.cpp` out of `/util`. These pure moves of entire files are very straight forward, so I feel like when we do this, we should do all of them at once as part of integrating the kernel library into the internal compilation units. My fear is that this will otherwise create to
...
(https://github.com/bitcoin/bitcoin/pull/27989#issuecomment-1611583183)
What about all the other networking and wallet related files in `/util`? E.g. we should probably also move `asmap.cpp`, `bip32.cpp`, `bytevectorhash.cpp`, `error.cpp`, `fees.cpp`, `syserror.cpp`, `message.cpp`, and `readwritefile.cpp` out of `/util`. These pure moves of entire files are very straight forward, so I feel like when we do this, we should do all of them at once as part of integrating the kernel library into the internal compilation units. My fear is that this will otherwise create to
...