💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1428379004)
too many zeroes on this, it's a 1/10 of a sat
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1428379004)
too many zeroes on this, it's a 1/10 of a sat
💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428387286)
I hadn't seen those PRs, thanks. I tried running `test_bitcoin` with the `TMPDIR` environment variable set (on Linux), and it works, so that means you don't have to set up a symlink.
> or.. don't delete anything and move such responsibility to the user.
Yes, I like that suggestion. I had forgotten that vscode (which is where I run the debugger from) allows one to set up an arbitrary "task" to run, `preLaunchTask`, before starting the executable, and this can remove the (fixed-named) data d
...
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428387286)
I hadn't seen those PRs, thanks. I tried running `test_bitcoin` with the `TMPDIR` environment variable set (on Linux), and it works, so that means you don't have to set up a symlink.
> or.. don't delete anything and move such responsibility to the user.
Yes, I like that suggestion. I had forgotten that vscode (which is where I run the debugger from) allows one to set up an arbitrary "task" to run, `preLaunchTask`, before starting the executable, and this can remove the (fixed-named) data d
...
💬 pablomartin4btc commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1858387980)
I've also tested the fix above provided by @hebasto and it works as expected, thanks!
Tested it running all tests successfully on Windows 11 Pro after a cross-build on Ubuntu 22.04 with the fix above.
```
.\test_bitcoin.exe --show_progress=true
Running 588 test cases...
0% 10 20 30 40 50 60 70 80 90 100%
|----|----|----|----|----|----|----|----|----|----|
***************************************************
*** No errors detected
```
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1858387980)
I've also tested the fix above provided by @hebasto and it works as expected, thanks!
Tested it running all tests successfully on Windows 11 Pro after a cross-build on Ubuntu 22.04 with the fix above.
```
.\test_bitcoin.exe --show_progress=true
Running 588 test cases...
0% 10 20 30 40 50 60 70 80 90 100%
|----|----|----|----|----|----|----|----|----|----|
***************************************************
*** No errors detected
```
💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428446668)
Sounds good to me.
Probably the only reason we may want to abort running a test on a specific datadir is when the directory contain mainnet data. But it shouldn't be a problem as we should be having something to detect this scenario inside `init.cpp` already.
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428446668)
Sounds good to me.
Probably the only reason we may want to abort running a test on a specific datadir is when the directory contain mainnet data. But it shouldn't be a problem as we should be having something to detect this scenario inside `init.cpp` already.
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1858463698)
Here's a small patch to add sigops-adjusted coverage for v3 children, with an intentional bug added to ensure it could be hit.
https://gist.github.com/instagibbs/c5cb0796ceec81f0374ae614f8cdab7f
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1858463698)
Here's a small patch to add sigops-adjusted coverage for v3 children, with an intentional bug added to ensure it could be hit.
https://gist.github.com/instagibbs/c5cb0796ceec81f0374ae614f8cdab7f
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1428467257)
> and C should be just "our highest block is older than 48h" (regardless of whether mapBlocksInFlight is empty) => then don't connect to limited peers.
Hmm, this observation makes me think..
what if `m_is_close_to_tip=true` and then the node lags behind; there could be an edge case scenario where the node starts slowly syncing-up the historical blocks before `CheckForStaleTipAndEvictPeers()` is executed, so `mapBlocksInFlight` wouldn't be empty, so.. it would still connect to limited peers e
...
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1428467257)
> and C should be just "our highest block is older than 48h" (regardless of whether mapBlocksInFlight is empty) => then don't connect to limited peers.
Hmm, this observation makes me think..
what if `m_is_close_to_tip=true` and then the node lags behind; there could be an edge case scenario where the node starts slowly syncing-up the historical blocks before `CheckForStaleTipAndEvictPeers()` is executed, so `mapBlocksInFlight` wouldn't be empty, so.. it would still connect to limited peers e
...
💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1428484311)
> Interesting. Try cleaning your working directory (e.g. `git clean -dfx`) and building again. To try on an older version of master, you can you check out any old commit and then do a `git cherry-pick ...`.
Git bisecting this I found that the tests start timing out with commit 4f473ea515bc77b9138323dab8a741c063d32e8f
Are you able to verify this on your setup?
@darosior would you be able to check as well? 🙏
This is how I did it:
```
git checkout 4f473ea515bc77b9138323dab8a741c063d
...
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1428484311)
> Interesting. Try cleaning your working directory (e.g. `git clean -dfx`) and building again. To try on an older version of master, you can you check out any old commit and then do a `git cherry-pick ...`.
Git bisecting this I found that the tests start timing out with commit 4f473ea515bc77b9138323dab8a741c063d32e8f
Are you able to verify this on your setup?
@darosior would you be able to check as well? 🙏
This is how I did it:
```
git checkout 4f473ea515bc77b9138323dab8a741c063d
...
💬 achow101 commented on pull request "tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858499111)
ACK 7b45744df33c6a4759eae1a3984f389cbac837c2
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858499111)
ACK 7b45744df33c6a4759eae1a3984f389cbac837c2
🚀 achow101 merged a pull request: "tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG"
(https://github.com/bitcoin/bitcoin/pull/29088)
(https://github.com/bitcoin/bitcoin/pull/29088)
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1858509374)
Good news, found something after all the walls of texts.
Having the ´TipMayBeStale()´ call surrounded by `GetUseAddrmanOutgoing()` check makes the node accept limited peers incoming connections indiscriminately (which is what we are trying to avoid here) when it was initialized with outgoing connections disabled.
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1858509374)
Good news, found something after all the walls of texts.
Having the ´TipMayBeStale()´ call surrounded by `GetUseAddrmanOutgoing()` check makes the node accept limited peers incoming connections indiscriminately (which is what we are trying to avoid here) when it was initialized with outgoing connections disabled.
💬 fjahr commented on pull request "refactor: C++20: Use lambda implicit capture and std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1858564296)
> Also, C++20 is enabled for years, so I don't understand the other commit.
What's the 'other' commit?
> I think you either forgot to compile locally, or your compiler doesn't emit any warnings that the changes are wrong?
I compiled locally just fine and ran all test using `Apple clang version 15.0.0` as the compiler. I will look into the warnings.
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1858564296)
> Also, C++20 is enabled for years, so I don't understand the other commit.
What's the 'other' commit?
> I think you either forgot to compile locally, or your compiler doesn't emit any warnings that the changes are wrong?
I compiled locally just fine and ran all test using `Apple clang version 15.0.0` as the compiler. I will look into the warnings.
🤔 mzumsande reviewed a pull request: "refactor: C++20: Use lambda implicit capture and std::rotl"
(https://github.com/bitcoin/bitcoin/pull/29085#pullrequestreview-1785029422)
I think the issue is that most of the spots that were changed don't actually make use of the implicit capture of `this` (e.g. by accessing a class or struct member in the body of the lambda) and therefore don't need to be changed.
Only the example in the scheduler does that (which is a doc). Not the use of `[=]` in general is deprecated, just using `[=]` in combination with accessing `this` via implicit capture.
(https://github.com/bitcoin/bitcoin/pull/29085#pullrequestreview-1785029422)
I think the issue is that most of the spots that were changed don't actually make use of the implicit capture of `this` (e.g. by accessing a class or struct member in the body of the lambda) and therefore don't need to be changed.
Only the example in the scheduler does that (which is a doc). Not the use of `[=]` in general is deprecated, just using `[=]` in combination with accessing `this` via implicit capture.
👍 TheCharlatan approved a pull request: "contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check"
(https://github.com/bitcoin/bitcoin/pull/28844#pullrequestreview-1785045706)
ACK ff896d25819da1c1e80591595c922fb093942645
Ran a guix build and:
```
nm -D bitcoind | grep -i gcc
...
U _Unwind_GetDataRelBase@GCC_3.0
U _Unwind_GetIPInfo@GCC_4.2.0
U _Unwind_GetLanguageSpecificData@GCC_3.0
...
```
(https://github.com/bitcoin/bitcoin/pull/28844#pullrequestreview-1785045706)
ACK ff896d25819da1c1e80591595c922fb093942645
Ran a guix build and:
```
nm -D bitcoind | grep -i gcc
...
U _Unwind_GetDataRelBase@GCC_3.0
U _Unwind_GetIPInfo@GCC_4.2.0
U _Unwind_GetLanguageSpecificData@GCC_3.0
...
```
💬 fjahr commented on pull request "refactor: C++20: Use lambda implicit capture and std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1858599666)
Seems right that the capture doesn't always need to be explicit, I dropped the second commit.
I don't see any performance impact on chacha20 from the first commit:
<details>
<summary>MASTER</summary>
$ src/bench/bench_bitcoin -filter="CHACHA20_1MB|CHACHA20_256BYTES|CHACHA20_64BYTES"
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1.92 | 522,177,61
...
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1858599666)
Seems right that the capture doesn't always need to be explicit, I dropped the second commit.
I don't see any performance impact on chacha20 from the first commit:
<details>
<summary>MASTER</summary>
$ src/bench/bench_bitcoin -filter="CHACHA20_1MB|CHACHA20_256BYTES|CHACHA20_64BYTES"
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1.92 | 522,177,61
...
💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428581458)
That's an excellent point, but I doubt there is anything already present that will prevent this.
Here are two ideas, what do you think?
- Whatever argument is given to `-datadir=`, _always_ make (if necessary) and use a subdirectory (within the specified directory) called `test_bitcoin`. That way, even if the user specifies their real data directory, all the test stuff will be within this subdirectory. (It's similar to how there already are subdirectories for testnet, signet, and regtest.)
...
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428581458)
That's an excellent point, but I doubt there is anything already present that will prevent this.
Here are two ideas, what do you think?
- Whatever argument is given to `-datadir=`, _always_ make (if necessary) and use a subdirectory (within the specified directory) called `test_bitcoin`. That way, even if the user specifies their real data directory, all the test stuff will be within this subdirectory. (It's similar to how there already are subdirectories for testnet, signet, and regtest.)
...
📝 1wiz1 opened a pull request: "Rename SECURITY.md to SECURITY.md."
(https://github.com/bitcoin/bitcoin/pull/29096)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/29096)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ fanquake closed a pull request: "Rename SECURITY.md to SECURITY.md."
(https://github.com/bitcoin/bitcoin/pull/29096)
(https://github.com/bitcoin/bitcoin/pull/29096)
📝 fanquake locked a pull request: "Rename SECURITY.md to SECURITY.md."
(https://github.com/bitcoin/bitcoin/pull/29096)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/29096)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1428713566)
> It would just be a one-line function to hold the code you already wrote in two places:
oh got it now.
> I wonder if this can be generalized to pass test-only options (along with a helper HasTestOption, similar to IsDeprecatedRPCEnabled?
good idea - liked how we can make sure users don't accidentally use these test-only options in their config file.
done.
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1428713566)
> It would just be a one-line function to hold the code you already wrote in two places:
oh got it now.
> I wonder if this can be generalized to pass test-only options (along with a helper HasTestOption, similar to IsDeprecatedRPCEnabled?
good idea - liked how we can make sure users don't accidentally use these test-only options in their config file.
done.
💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1428713673)
> I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.
it would be cleaner to not need to know the existing context of addrman when dealing with getaddrmaninfo/getrawaddrman tests. both the options sound good to me!
> The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in the unit test). This would leave us with quite a few additional addresses already in the tried tab
...
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1428713673)
> I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.
it would be cleaner to not need to know the existing context of addrman when dealing with getaddrmaninfo/getrawaddrman tests. both the options sound good to me!
> The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in the unit test). This would leave us with quite a few additional addresses already in the tried tab
...