✅ maflcko closed an issue: "Single-Glyph Bitcoin Transaction System"
(https://github.com/bitcoin/bitcoin/issues/31568)
(https://github.com/bitcoin/bitcoin/issues/31568)
💬 maflcko commented on issue "Single-Glyph Bitcoin Transaction System":
(https://github.com/bitcoin/bitcoin/issues/31568#issuecomment-2568233702)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.
Network-wide consensus and/or P2P changes first need to be discussed with the greater ecosystem, for example https://groups.google.c
...
(https://github.com/bitcoin/bitcoin/issues/31568#issuecomment-2568233702)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.
Network-wide consensus and/or P2P changes first need to be discussed with the greater ecosystem, for example https://groups.google.c
...
💬 maflcko commented on issue "failure in wallet_sendall.py --descriptors":
(https://github.com/bitcoin/bitcoin/issues/31571#issuecomment-2568254664)
Did you run any other command while running the tests?
Does it reproduce?
I can't reproduce with your steps on Ubuntu 22.04: `cmake -B ./bld-cmake -DBUILD_GUI=OFF -DWITH_BDB=OFF -DBUILD_BENCH=OFF -DBUILD_FOR_FUZZING=OFF -DBUILD_KERNEL_LIB=OFF -DBUILD_SHARED_LIBS=OFF -DBUILD_TESTS=OFF -DBUILD_TX=OFF -DBUILD_UTIL=OFF -DBUILD_WALLET_TOOL=OFF && cmake --build ./bld-cmake --parallel $(nproc) && ./bld-cmake/test/functional/test_runner.py`
(https://github.com/bitcoin/bitcoin/issues/31571#issuecomment-2568254664)
Did you run any other command while running the tests?
Does it reproduce?
I can't reproduce with your steps on Ubuntu 22.04: `cmake -B ./bld-cmake -DBUILD_GUI=OFF -DWITH_BDB=OFF -DBUILD_BENCH=OFF -DBUILD_FOR_FUZZING=OFF -DBUILD_KERNEL_LIB=OFF -DBUILD_SHARED_LIBS=OFF -DBUILD_TESTS=OFF -DBUILD_TX=OFF -DBUILD_UTIL=OFF -DBUILD_WALLET_TOOL=OFF && cmake --build ./bld-cmake --parallel $(nproc) && ./bld-cmake/test/functional/test_runner.py`
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901189022)
It does not. I'll remove it
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901189022)
It does not. I'll remove it
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901192156)
I've cached `m_outbounds_count` to prevent this from happening.
This made me realize something that `fanout_targets` was incorrectly initialized, instead of reserving space for `inbounds_target_size + outbounds_target_size)`, it was being initialized with `inbounds_target_size + outbounds_target_size)` pre-existing elements initialized to 0, meaning that the returned vector contained more elements that expected
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901192156)
I've cached `m_outbounds_count` to prevent this from happening.
This made me realize something that `fanout_targets` was incorrectly initialized, instead of reserving space for `inbounds_target_size + outbounds_target_size)`, it was being initialized with `inbounds_target_size + outbounds_target_size)` pre-existing elements initialized to 0, meaning that the returned vector contained more elements that expected
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901195650)
Addressed in 804ccf030362fe5b232fe5c4213b77899a425f2b
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901195650)
Addressed in 804ccf030362fe5b232fe5c4213b77899a425f2b
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901195715)
Addressed in 804ccf030362fe5b232fe5c4213b77899a425f2b
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901195715)
Addressed in 804ccf030362fe5b232fe5c4213b77899a425f2b
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901208893)
This is already the case. The transaction that is passed to `RelayTransaction` is added to `invs_to_send` using `wtxid/txid` based on `m_wtxid_relay` (that is part of the code you quoted already). For other transactions that may be in `inv_to_send` (e.g. ancestors and/or descendants), this only apply to Erlay, and Erlay enforces `WTXID_RELAY`
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901208893)
This is already the case. The transaction that is passed to `RelayTransaction` is added to `invs_to_send` using `wtxid/txid` based on `m_wtxid_relay` (that is part of the code you quoted already). For other transactions that may be in `inv_to_send` (e.g. ancestors and/or descendants), this only apply to Erlay, and Erlay enforces `WTXID_RELAY`
💬 edilmedeiros commented on issue "Source code mapping for debugger has changed since cmake":
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-2568291604)
After many hours, I could make it work.
Add this to ~/.lldbinit to make it load a local .lldbinit file: `settings set target.load-cwd-lldbinit true`. This is a convenience so that I can keep everything related to Bitcoin Core in its repo.
In the Bitcoin Core repo folder, add a .lldbinit file with (mine is located at `/Users/jose.edil/2-development/bitcoin/bitcoin-core-dev`, adjust to your path):
```
settings set target.source-map /Users/jose.edil/2-development/bitcoin/bitcoin-core-dev/bu
...
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-2568291604)
After many hours, I could make it work.
Add this to ~/.lldbinit to make it load a local .lldbinit file: `settings set target.load-cwd-lldbinit true`. This is a convenience so that I can keep everything related to Bitcoin Core in its repo.
In the Bitcoin Core repo folder, add a .lldbinit file with (mine is located at `/Users/jose.edil/2-development/bitcoin/bitcoin-core-dev`, adjust to your path):
```
settings set target.source-map /Users/jose.edil/2-development/bitcoin/bitcoin-core-dev/bu
...
💬 maflcko commented on pull request "[28.x] 28.1 backports (final?)":
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2568299441)
> Are these significant enough to warrant a rc3 or can we go straight to final?
Yes, seems fine in this special case, as the depends patch shouldn't affect the release bin and the other change is test-only. I went ahead and picked your commits from https://github.com/bitcoin/bitcoin/pull/31582
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2568299441)
> Are these significant enough to warrant a rc3 or can we go straight to final?
Yes, seems fine in this special case, as the depends patch shouldn't affect the release bin and the other change is test-only. I went ahead and picked your commits from https://github.com/bitcoin/bitcoin/pull/31582
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901215478)
The rationale for who this method is used is provided in `RelayTransaction` when the method is used
https://github.com/bitcoin/bitcoin/blob/804ccf030362fe5b232fe5c4213b77899a425f2b/src/net_processing.cpp#L2154-L2159
I think the docs give enough rationale as to what this is intended for without being excessively narrow as to what the use cases could be.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901215478)
The rationale for who this method is used is provided in `RelayTransaction` when the method is used
https://github.com/bitcoin/bitcoin/blob/804ccf030362fe5b232fe5c4213b77899a425f2b/src/net_processing.cpp#L2154-L2159
I think the docs give enough rationale as to what this is intended for without being excessively narrow as to what the use cases could be.
💬 brunoerg commented on issue "Fuzz: Runtime errors when running fuzz tests on MacOs":
(https://github.com/bitcoin/bitcoin/issues/31591#issuecomment-2568305291)
I have the same issue and I think other people also have reported it before, I simply ignore it.
(https://github.com/bitcoin/bitcoin/issues/31591#issuecomment-2568305291)
I have the same issue and I think other people also have reported it before, I simply ignore it.
💬 marcofleon commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2568305724)
> I am unable to crash the fuzz test when `setmocktime()` is removed from one of the targets.
Which target was it?
It could be that running the test from scratch doesn't quickly reach code where `now()` is called. Try running the target on a corpus of inputs:
```
FUZZ=load_external_block_file ./mocktimefuzzbuild/src/test/fuzz/fuzz ../qa-assets/fuzz_corpora/load_external_block_file -runs=1
```
You can find the corpora here: https://github.com/bitcoin-core/qa-assets
I think it's also
...
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2568305724)
> I am unable to crash the fuzz test when `setmocktime()` is removed from one of the targets.
Which target was it?
It could be that running the test from scratch doesn't quickly reach code where `now()` is called. Try running the target on a corpus of inputs:
```
FUZZ=load_external_block_file ./mocktimefuzzbuild/src/test/fuzz/fuzz ../qa-assets/fuzz_corpora/load_external_block_file -runs=1
```
You can find the corpora here: https://github.com/bitcoin-core/qa-assets
I think it's also
...
💬 marcofleon commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#discussion_r1901223667)
Thanks, yeah that makes more sense. This actually caught system time usage in `p2p_headers_presync` that wasn't crashing before. Because the `mocktime.count()` check is now in the function itself rather than at the end of the fuzz iteration.
Using that syntax didn't compile for me, but I believe what I just pushed should work.
(https://github.com/bitcoin/bitcoin/pull/31549#discussion_r1901223667)
Thanks, yeah that makes more sense. This actually caught system time usage in `p2p_headers_presync` that wasn't crashing before. Because the `mocktime.count()` check is now in the function itself rather than at the end of the fuzz iteration.
Using that syntax didn't compile for me, but I believe what I just pushed should work.
🤔 mzumsande reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2521245377)
Thanks - your explanation makes sense - I did a bit of research on how much logic and I/O in constructors are considered to be "good style", and there doesn't really seem to be a clear consensus to me.
For me the main downside would be that some of the things the ctor does may be unexpected, hence below suggestion to add some documentation.
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2521245377)
Thanks - your explanation makes sense - I did a bit of research on how much logic and I/O in constructors are considered to be "good style", and there doesn't really seem to be a clear consensus to me.
For me the main downside would be that some of the things the ctor does may be unexpected, hence below suggestion to add some documentation.
💬 mzumsande commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901166334)
`cache_sizes` is no longer used and can be removed from `CompleteChainstateInitialization()`
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901166334)
`cache_sizes` is no longer used and can be removed from `CompleteChainstateInitialization()`
💬 mzumsande commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901165977)
How about a comment here to document the non-trivial things this does (creating blockman, opening blocksdb, cleaning up old block files in case of pruning) and doesn't (opening coins db)? I think this would help understanding the sequence better.
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901165977)
How about a comment here to document the non-trivial things this does (creating blockman, opening blocksdb, cleaning up old block files in case of pruning) and doesn't (opening coins db)? I think this would help understanding the sequence better.
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901245990)
Calling `RegisterPeer` with the same `peer_id` multiple times will result in `ReconciliationRegisterResult::ALREADY_REGISTERED`.
The docs of both `PreRegisterPeer` and `RegisterPeer` specify that they should called only once.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901245990)
Calling `RegisterPeer` with the same `peer_id` multiple times will result in `ReconciliationRegisterResult::ALREADY_REGISTERED`.
The docs of both `PreRegisterPeer` and `RegisterPeer` specify that they should called only once.
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901246821)
Added in [b84348a](https://github.com/bitcoin/bitcoin/pull/30116/commits/b84348a8fc93c86dcefb936f986ff2f1ced320b4)
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901246821)
Added in [b84348a](https://github.com/bitcoin/bitcoin/pull/30116/commits/b84348a8fc93c86dcefb936f986ff2f1ced320b4)
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901247541)
Addressed in 0e290996874200317ba4d3f1b3e12992b40db4d1
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901247541)
Addressed in 0e290996874200317ba4d3f1b3e12992b40db4d1