💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627952167)
Did you really push it? https://github.com/bitcoin/bitcoin/compare/a92e28e852f213c522df9858e240470299e4ecaa..471d27d5d740f82fd9f51a6e909a8ae3bdd75828 says no changes.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627952167)
Did you really push it? https://github.com/bitcoin/bitcoin/compare/a92e28e852f213c522df9858e240470299e4ecaa..471d27d5d740f82fd9f51a6e909a8ae3bdd75828 says no changes.
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627952472)
Ok, fair enough.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1627952472)
Ok, fair enough.
💬 brunoerg commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1627954290)
Why is it calling `Connect` without specifying `to`?
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1627954290)
Why is it calling `Connect` without specifying `to`?
💬 fanquake commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2150331121)
https://github.com/bitcoin/bitcoin/pull/30232/checks?check_run_id=25845075037:
```bash
A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.
```
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2150331121)
https://github.com/bitcoin/bitcoin/pull/30232/checks?check_run_id=25845075037:
```bash
A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.
```
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2150332074)
The idea of saving heights is interesting to me because wallet code assumes it know block heights many of places, but it doesn't actually store heights anywhere. So for example, if there wallet stored a mapping of block hashes to heights (or other block information) it might be useful for allowing the wallet to work in an offline mode or letting the wallet CLI tool have more capabilities. This is all pretty abstract though, so I'm not suggesting something specific.
> Perhaps an easy solution
...
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2150332074)
The idea of saving heights is interesting to me because wallet code assumes it know block heights many of places, but it doesn't actually store heights anywhere. So for example, if there wallet stored a mapping of block hashes to heights (or other block information) it might be useful for allowing the wallet to work in an offline mode or letting the wallet CLI tool have more capabilities. This is all pretty abstract though, so I'm not suggesting something specific.
> Perhaps an easy solution
...
💬 fanquake commented on pull request "guix: bump time-machine to f0bb724211872cd6158fce6162e0b8c73efed126":
(https://github.com/bitcoin/bitcoin/pull/30231#issuecomment-2150426967)
Guix Build (aarch64):
```bash
766de337912412943b38f6f0dd100bafaa858bf2ad43bbc56de9de86b59f99c9 guix-build-2599655c1fb8/output/aarch64-linux-gnu/SHA256SUMS.part
a426d7189067919d418e164b5770c580bc6e94c3aa5320b2980c65762055ec92 guix-build-2599655c1fb8/output/aarch64-linux-gnu/bitcoin-2599655c1fb8-aarch64-linux-gnu-debug.tar.gz
c70f267474be9121d70e217c966c6c359ff12efb369a98e787dabc63221e2a47 guix-build-2599655c1fb8/output/aarch64-linux-gnu/bitcoin-2599655c1fb8-aarch64-linux-gnu.tar.gz
75038a
...
(https://github.com/bitcoin/bitcoin/pull/30231#issuecomment-2150426967)
Guix Build (aarch64):
```bash
766de337912412943b38f6f0dd100bafaa858bf2ad43bbc56de9de86b59f99c9 guix-build-2599655c1fb8/output/aarch64-linux-gnu/SHA256SUMS.part
a426d7189067919d418e164b5770c580bc6e94c3aa5320b2980c65762055ec92 guix-build-2599655c1fb8/output/aarch64-linux-gnu/bitcoin-2599655c1fb8-aarch64-linux-gnu-debug.tar.gz
c70f267474be9121d70e217c966c6c359ff12efb369a98e787dabc63221e2a47 guix-build-2599655c1fb8/output/aarch64-linux-gnu/bitcoin-2599655c1fb8-aarch64-linux-gnu.tar.gz
75038a
...
💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2150437453)
ACK e4ecb8217ada3dae1c1645a8d0a12e14b0f935da
verified fuzz target coverage and sensible invariant checks are being enforced
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2150437453)
ACK e4ecb8217ada3dae1c1645a8d0a12e14b0f935da
verified fuzz target coverage and sensible invariant checks are being enforced
⚠️ fanquake reopened an issue: "brew: serfloat_tests tests fail on Linux"
(https://github.com/bitcoin/bitcoin/issues/28941)
While upgrading sqlite to 3.44.1, we've found some regression test failure
<details>
<summary>test failure log</summary>
```
==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/25.1/bin/test_bitcoin
Running 557 test cases...
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [3933824172291540 != 0]
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1522707584769707 != 0]
t
...
(https://github.com/bitcoin/bitcoin/issues/28941)
While upgrading sqlite to 3.44.1, we've found some regression test failure
<details>
<summary>test failure log</summary>
```
==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/25.1/bin/test_bitcoin
Running 557 test cases...
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [3933824172291540 != 0]
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1522707584769707 != 0]
t
...
💬 fanquake commented on issue "brew: serfloat_tests tests fail on Linux":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-2150440807)
Reopening as this is still failing, see: https://github.com/Homebrew/homebrew-core/pull/169269#issuecomment-2150438892.
Can be reproduced with:
```bash
docker pull docker.io/homebrew/brew:latest
docker run -it homebrew/brew
brew install bitcoin --head --debug # ignore the 2 patch failures
brew test bitcoin
/home/linuxbrew/.linuxbrew/Cellar/bitcoin/HEAD-ff7d205/bin/test_bitcoin
Running 610 test cases...
test/serfloat_tests.cpp(95): error: in "serfloat_tests/double_serfloat_tests":
...
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-2150440807)
Reopening as this is still failing, see: https://github.com/Homebrew/homebrew-core/pull/169269#issuecomment-2150438892.
Can be reproduced with:
```bash
docker pull docker.io/homebrew/brew:latest
docker run -it homebrew/brew
brew install bitcoin --head --debug # ignore the 2 patch failures
brew test bitcoin
/home/linuxbrew/.linuxbrew/Cellar/bitcoin/HEAD-ff7d205/bin/test_bitcoin
Running 610 test cases...
test/serfloat_tests.cpp(95): error: in "serfloat_tests/double_serfloat_tests":
...
💬 dergoegge commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1627820274)
```suggestion
// Copyright (c) The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
```
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1627820274)
```suggestion
// Copyright (c) The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
```
💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1628090346)
Done (through `.rbegin()` instead as your `std::reverse_iterate` suggestion didn't compile).
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1628090346)
Done (through `.rbegin()` instead as your `std::reverse_iterate` suggestion didn't compile).
📝 sr-gi opened a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233)
`m_is_inbound` cannot be changed throughout the life of a `Peer`. However, we are currently storing it in `CNodeState`, which requires locking `cs_main` in order to access it. This can be moved to the outside scope and only require `m_peer_mutex`.
This is a refactor in preparation for Erlay reworks.
<!--
*** 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
...
(https://github.com/bitcoin/bitcoin/pull/30233)
`m_is_inbound` cannot be changed throughout the life of a `Peer`. However, we are currently storing it in `CNodeState`, which requires locking `cs_main` in order to access it. This can be moved to the outside scope and only require `m_peer_mutex`.
This is a refactor in preparation for Erlay reworks.
<!--
*** 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
...
💬 luke-jr commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2150551869)
>A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.
How do you propose resolving this? It's not really a circular dependency, just equivocated as such due to the CI test stripping file extensions, but maybe there's a better approach that just fixing CI?
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2150551869)
>A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.
How do you propose resolving this? It's not really a circular dependency, just equivocated as such due to the CI test stripping file extensions, but maybe there's a better approach that just fixing CI?
👍 ryanofsky approved a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2099552897)
Code review ACK 9617e42a7b12a05ed8a0815c5a6537e1614727f5. I didn't follow all the previous discussion, but I like the place this PR ended up accepting simple "owner" "group" and "all" settings and avoiding more complicated corner cases.
I left some suggestions which are mostly not important, though I do think it would be better to try to stay more backwards compatible and avoid setting permissions when it is not requested.
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2099552897)
Code review ACK 9617e42a7b12a05ed8a0815c5a6537e1614727f5. I didn't follow all the previous discussion, but I like the place this PR ended up accepting simple "owner" "group" and "all" settings and avoiding more complicated corner cases.
I left some suggestions which are mostly not important, though I do think it would be better to try to stay more backwards compatible and avoid setting permissions when it is not requested.
💬 ryanofsky commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1627997774)
In commit "rpc: add default 600 permissions for cookie file" (35df85346438258e1bb5f736e9c4d48cf59e6547)
This commit message is confusing because it sounds like it changing the permissions of the cookie file, but the diff only shows it adding a constant declaration and not changing any code?
It seems like it would be better if this commit were combined with the third commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a) where the new constant is actu
...
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1627997774)
In commit "rpc: add default 600 permissions for cookie file" (35df85346438258e1bb5f736e9c4d48cf59e6547)
This commit message is confusing because it sounds like it changing the permissions of the cookie file, but the diff only shows it adding a constant declaration and not changing any code?
It seems like it would be better if this commit were combined with the third commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a) where the new constant is actu
...
💬 ryanofsky commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1628110701)
In commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a)
I think it would be good to change `fs::perms` parameter to `std::optional<fs::perms>` and not call `fs::permissions()` unless `-rpccookieperms` option is specified, for backwards compatibility.
It is easy to imagine a new fs::permissions(DEFAULT_COOKIE_PERMS) call having bad side effects on platforms like windows that use acls, or maybe failing on filesystems that do not support permissions.
...
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1628110701)
In commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a)
I think it would be good to change `fs::perms` parameter to `std::optional<fs::perms>` and not call `fs::permissions()` unless `-rpccookieperms` option is specified, for backwards compatibility.
It is easy to imagine a new fs::permissions(DEFAULT_COOKIE_PERMS) call having bad side effects on platforms like windows that use acls, or maybe failing on filesystems that do not support permissions.
...
💬 ryanofsky commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1628108133)
In commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a)
It would be nice to drop this ifdef special case for windows. Even though windows uses acls, the fs::permissions call should still be able to do the requested things by setting acls for the "Users" and "Everyone" groups on windows. I don't think we need to go out of our way to second guess the library and operating system here. If the operating system or filesystem do not support permissions, or
...
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1628108133)
In commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a)
It would be nice to drop this ifdef special case for windows. Even though windows uses acls, the fs::permissions call should still be able to do the requested things by setting acls for the "Users" and "Everyone" groups on windows. I don't think we need to go out of our way to second guess the library and operating system here. If the operating system or filesystem do not support permissions, or
...
🤔 murchandamus reviewed a pull request: "wallet: re-activate "AmountWithFeeExceedsBalance" error"
(https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-2099857752)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-2099857752)
Concept ACK
💬 murchandamus commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r1628175898)
It seems that both in lines 1137 and 1139 the preset_inputs are counted with their full amount rather than their effective amount. Should the `preset_inputs` not also be reduced to their effective amount in line 1139?
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r1628175898)
It seems that both in lines 1137 and 1139 the preset_inputs are counted with their full amount rather than their effective amount. Should the `preset_inputs` not also be reduced to their effective amount in line 1139?
📝 theuni opened a pull request: "Enable clang-tidy checks for self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30234)
See comment here: https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148229582
Our code failed these checks in two places, which have been fixed up here. Though these appear to have been harmless, adding the check avoids the copy in the self-assignment case so there should be no downside.
Additionally, minisketch failed the check as well. See https://github.com/sipa/minisketch/pull/87
After fixing up the violations, turn on the aggressive clang-tidy check.
Note that I only l
...
(https://github.com/bitcoin/bitcoin/pull/30234)
See comment here: https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148229582
Our code failed these checks in two places, which have been fixed up here. Though these appear to have been harmless, adding the check avoids the copy in the self-assignment case so there should be no downside.
Additionally, minisketch failed the check as well. See https://github.com/sipa/minisketch/pull/87
After fixing up the violations, turn on the aggressive clang-tidy check.
Note that I only l
...