🤔 murchandamus reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-1977655865)
Did a quick pass. I am surprised we are able to get this big of an improvement without p2p changes, seems like a big win. Big Concept ACK, but I must admit this part of the codebase is a bit out of my wheelhouse.
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-1977655865)
Did a quick pass. I am surprised we are able to get this big of an improvement without p2p changes, seems like a big win. Big Concept ACK, but I must admit this part of the codebase is a bit out of my wheelhouse.
💬 murchandamus commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550334355)
Nit: Could use [`std::set::contains`](https://en.cppreference.com/w/cpp/container/set/contains) here and below
```suggestion
BOOST_CHECK(expected_parent1_children.contains(child->GetWitnessHash()));
```
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550334355)
Nit: Could use [`std::set::contains`](https://en.cppreference.com/w/cpp/container/set/contains) here and below
```suggestion
BOOST_CHECK(expected_parent1_children.contains(child->GetWitnessHash()));
```
💬 murchandamus commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550260907)
In guard against MempoolAcceptResult::m_replaced_transactions (53f1e65f30a0a6b931e97743113e0227748680df):
I am not well-acquainted with `net_processing.cpp`, but I figured I could still mention that it is unclear to me from the commit message and the code change how this change fits in the context. Were we previously assuming that we would always have a non-empty list for `m_replaced_transactions` in the context of this call?
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550260907)
In guard against MempoolAcceptResult::m_replaced_transactions (53f1e65f30a0a6b931e97743113e0227748680df):
I am not well-acquainted with `net_processing.cpp`, but I figured I could still mention that it is unclear to me from the commit message and the code change how this change fits in the context. Were we previously assuming that we would always have a non-empty list for `m_replaced_transactions` in the context of this call?
💬 theuni commented on issue "Compilation failure when using `--enable-fuzz` and `--enable-debug` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035485312)
In addition to the discussion in #29781, I'll PR a change to make this work with `DISABLE_OPTIMIZED_SHA256` as a fallback. I'm doing that as part of a larger refactor of the crypto defines, though.
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035485312)
In addition to the discussion in #29781, I'll PR a change to make this work with `DISABLE_OPTIMIZED_SHA256` as a fallback. I'm doing that as part of a larger refactor of the crypto defines, though.
💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550398421)
@murchandamus: I was asking myself the same a few days ago and started with some review notes for each commit. The one for 53f1e65f30a0a6b931e97743113e0227748680df might fit to your question (note that it's not about empty vs. non-empty, but more about set-to-nothing vs. set-to-something, since it's an std::optional):
```
The only way to create an ATMP result of type `MempoolAcceptResult::ResultType::VALID` is using the
static method `MempoolAccepptResult::Success`, which in turn calls the pr
...
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550398421)
@murchandamus: I was asking myself the same a few days ago and started with some review notes for each commit. The one for 53f1e65f30a0a6b931e97743113e0227748680df might fit to your question (note that it's not about empty vs. non-empty, but more about set-to-nothing vs. set-to-something, since it's an std::optional):
```
The only way to create an ATMP result of type `MempoolAcceptResult::ResultType::VALID` is using the
static method `MempoolAccepptResult::Success`, which in turn calls the pr
...
💬 achow101 commented on pull request "rpc: Optimize serialization disk space of dumptxoutset - Reloaded":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2035502334)
> Do you know of any examples of the utxo dumps already being used for something else?
For myself, I have a couple of random scripts that read utxo dumps in order to compute some statistics and other info on the utxo set. I'm not aware of any actual projects using utxo snapshots, but I also haven't actively looked.
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2035502334)
> Do you know of any examples of the utxo dumps already being used for something else?
For myself, I have a couple of random scripts that read utxo dumps in order to compute some statistics and other info on the utxo set. I'm not aware of any actual projects using utxo snapshots, but I also haven't actively looked.
💬 emc99 commented on pull request "guix: remove errant leftover from #29648":
(https://github.com/bitcoin/bitcoin/pull/29787#issuecomment-2035514900)
> ACK [fd8527a](https://github.com/bitcoin/bitcoin/commit/fd8527a20ebc490df030b3a91c1161f00c8a29b6)
>
> Verified the failure, reviewed the change and running a guix build now without any issues (will post the results once it's finished).
When you say you are running a guix build, do you mean you are running the guix operating system on a separate machine?
(https://github.com/bitcoin/bitcoin/pull/29787#issuecomment-2035514900)
> ACK [fd8527a](https://github.com/bitcoin/bitcoin/commit/fd8527a20ebc490df030b3a91c1161f00c8a29b6)
>
> Verified the failure, reviewed the change and running a guix build now without any issues (will post the results once it's finished).
When you say you are running a guix build, do you mean you are running the guix operating system on a separate machine?
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550445308)
> it is unclear to me from the commit message and the code change how this change fits in the context.
(Note that this commit is a followup from #29619)
Yes, it should always have a value when the result is VALID. This is just adding a belt-and-suspenders juuust in case.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550445308)
> it is unclear to me from the commit message and the code change how this change fits in the context.
(Note that this commit is a followup from #29619)
Yes, it should always have a value when the result is VALID. This is just adding a belt-and-suspenders juuust in case.
💬 TheCharlatan commented on pull request "guix: remove errant leftover from #29648":
(https://github.com/bitcoin/bitcoin/pull/29787#issuecomment-2035556508)
Re https://github.com/bitcoin/bitcoin/pull/29787#issuecomment-2035514900
> When you say you are running a guix build, do you mean you are running the guix operating system on a separate machine?
Guix builds describe this project's method for compiling reproducible binaries (see https://reproducible-builds.org/). It is the same method that is used for Bitcoin Core release binaries. Guix is the package and build environment manager used for this process. In this context guix is installed on
...
(https://github.com/bitcoin/bitcoin/pull/29787#issuecomment-2035556508)
Re https://github.com/bitcoin/bitcoin/pull/29787#issuecomment-2035514900
> When you say you are running a guix build, do you mean you are running the guix operating system on a separate machine?
Guix builds describe this project's method for compiling reproducible binaries (see https://reproducible-builds.org/). It is the same method that is used for Bitcoin Core release binaries. Guix is the package and build environment manager used for this process. In this context guix is installed on
...
💬 murchandamus commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550494056)
Thanks! Great, I’ll attempt another more thorough review at a later time.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550494056)
Thanks! Great, I’ll attempt another more thorough review at a later time.
💬 kosuodhmwa commented on issue "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?":
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2035593511)
Now i saw that i also have message like that:
`2024-04-03T20:43:47Z Socks5() connect to 77.7.121.181:8333 failed: general failure`
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2035593511)
Now i saw that i also have message like that:
`2024-04-03T20:43:47Z Socks5() connect to 77.7.121.181:8333 failed: general failure`
💬 kosuodhmwa commented on issue "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?":
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2035617849)
**_"Do you get only IP addresses in those messages (x.x.x.x) or also onion addresses (e.g. ydvbxdzs6w5wefifiqsqntpbd7tliofenqih5hlnz34546fvy4ab7iid.onion)?"_**
No, only for IP addresses as shown on post above. @vasild
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2035617849)
**_"Do you get only IP addresses in those messages (x.x.x.x) or also onion addresses (e.g. ydvbxdzs6w5wefifiqsqntpbd7tliofenqih5hlnz34546fvy4ab7iid.onion)?"_**
No, only for IP addresses as shown on post above. @vasild
💬 cbergqvist commented on pull request "test: Bump timeouts in feature_index_prune and wallet_importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2035711744)
Thanks for having a look @tdb3!
As mentioned in the summary, running without `--jobs=16` makes the tests succeed. I guess it is not a common parameter (even less common in conjunction with `--extended`).
Your questions prompted me to perform the tests on older release branches to see if behavior changed recently. (No other tests failed but I didn't let `feature_dbcrash.py` and complete since it takes around 45 minutes (occasionally skipped some other stragglers as well)).
*27.x* - Only
...
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2035711744)
Thanks for having a look @tdb3!
As mentioned in the summary, running without `--jobs=16` makes the tests succeed. I guess it is not a common parameter (even less common in conjunction with `--extended`).
Your questions prompted me to perform the tests on older release branches to see if behavior changed recently. (No other tests failed but I didn't let `feature_dbcrash.py` and complete since it takes around 45 minutes (occasionally skipped some other stragglers as well)).
*27.x* - Only
...
🤔 pablomartin4btc requested changes to a pull request: "Don't permit port in proxy IP option"
(https://github.com/bitcoin-core/gui/pull/813#pullrequestreview-1978210897)
Concept ACK 94173eac6e89f374f1a98b5d0e449a0a07ed832d
I'd suggest to use a regex validator for the IP address, it's much simpler I think and the user won't be allow to enter any other symbols or chars (typo?) that aren't digits (plus user can't type more than 3 subnets separated by dots where currently you could and any symbols):
```diff
diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
index dd654a7ab..1fabee7f2 100644
--- a/src/qt/optionsdialog.cpp
+++ b/src/qt/optionsd
...
(https://github.com/bitcoin-core/gui/pull/813#pullrequestreview-1978210897)
Concept ACK 94173eac6e89f374f1a98b5d0e449a0a07ed832d
I'd suggest to use a regex validator for the IP address, it's much simpler I think and the user won't be allow to enter any other symbols or chars (typo?) that aren't digits (plus user can't type more than 3 subnets separated by dots where currently you could and any symbols):
```diff
diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
index dd654a7ab..1fabee7f2 100644
--- a/src/qt/optionsdialog.cpp
+++ b/src/qt/optionsd
...
💬 cbergqvist commented on pull request "test: Bump timeouts in feature_index_prune and wallet_importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2035716197)
@itornaza subtly prompted me to do some tests to find the sweet spot for the number of jobs on my hardware, which luckily helps motivate why running with 16 jobs is not unreasonable.
This is wall time running on a [RAM disk](https://github.com/bitcoin/bitcoin/blob/master/test/README.md#speed-up-test-runs-with-a-ram-disk) **without `--extended`**.
```
jobs duration (s) results
4 816 all passed
8 454 all passed
12 342 all passed
16 300 all pas
...
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2035716197)
@itornaza subtly prompted me to do some tests to find the sweet spot for the number of jobs on my hardware, which luckily helps motivate why running with 16 jobs is not unreasonable.
This is wall time running on a [RAM disk](https://github.com/bitcoin/bitcoin/blob/master/test/README.md#speed-up-test-runs-with-a-ram-disk) **without `--extended`**.
```
jobs duration (s) results
4 816 all passed
8 454 all passed
12 342 all passed
16 300 all pas
...
💬 fjahr commented on pull request "ThreadSanitizer: Fix #29767":
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2035727894)
Code review ACK bbe82c116e72ca0638751e063bf564cd1fe5c4d5
This looks like the correct fix to me. CI failure is unrelated.
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2035727894)
Code review ACK bbe82c116e72ca0638751e063bf564cd1fe5c4d5
This looks like the correct fix to me. CI failure is unrelated.
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#issuecomment-2035728966)
Added a commit so that the fuzzer will skip the type of inputs that was causing the previous failure. This should be safe as that failure is not reachable in normal usage. It can only be reached if the user's wallet is corrupted, and in that case, the runtime error would propagate up to them so there would not be a crash.
(https://github.com/bitcoin/bitcoin/pull/28333#issuecomment-2035728966)
Added a commit so that the fuzzer will skip the type of inputs that was causing the previous failure. This should be safe as that failure is not reachable in normal usage. It can only be reached if the user's wallet is corrupted, and in that case, the runtime error would propagate up to them so there would not be a crash.
✅ GopherJ closed an issue: "Always exit silently"
(https://github.com/bitcoin/bitcoin/issues/29783)
(https://github.com/bitcoin/bitcoin/issues/29783)
💬 GopherJ commented on issue "Always exit silently":
(https://github.com/bitcoin/bitcoin/issues/29783#issuecomment-2035808444)
closing as it's now more related to system instead of bitcoin. thanks for your help!
(https://github.com/bitcoin/bitcoin/issues/29783#issuecomment-2035808444)
closing as it's now more related to system instead of bitcoin. thanks for your help!
💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#discussion_r1550751325)
Thank you for reviewing. **Would you be ok with a simplified approach of writing all unit test output to stdout, with this output being shown when unit tests fail?** This seems a bit cleaner and more consistent than selectively placing all unit test output to stdout or stderr depending on test pass/fail.
### More info
`TextTestRunner` lets us print unit test output to one stream. Previously (before this PR), `TextTestRunner` wrote all output to stderr regardless of pass/fail but doing t
...
(https://github.com/bitcoin/bitcoin/pull/29771#discussion_r1550751325)
Thank you for reviewing. **Would you be ok with a simplified approach of writing all unit test output to stdout, with this output being shown when unit tests fail?** This seems a bit cleaner and more consistent than selectively placing all unit test output to stdout or stderr depending on test pass/fail.
### More info
`TextTestRunner` lets us print unit test output to one stream. Previously (before this PR), `TextTestRunner` wrote all output to stderr regardless of pass/fail but doing t
...