💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1771249650)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1771249650)
Done, thanks
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2367971650)
> set `CCACHE_COMPILERCHECK` to `content` mode, which passes the compiler check when the hash of the compiler is the same (the default is `mdate` which will change with each container we create)
Are you sure? At least on Ubuntu Noble `mdate` is set correctly for `clang` (and does not change with each container). I presume the same is true for Debian and GCC. So, if the `modify` is different, the content would be different as well (and vice-versa). See:
```
# stat /usr/bin/clang++
File:
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2367971650)
> set `CCACHE_COMPILERCHECK` to `content` mode, which passes the compiler check when the hash of the compiler is the same (the default is `mdate` which will change with each container we create)
Are you sure? At least on Ubuntu Noble `mdate` is set correctly for `clang` (and does not change with each container). I presume the same is true for Debian and GCC. So, if the `modify` is different, the content would be different as well (and vice-versa). See:
```
# stat /usr/bin/clang++
File:
...
💬 l0rinc commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2368014082)
> Didn’t #28280 resolve this?
That's why I was checking it, I've rebased it on [9cb9651d92ddb5d92724f6a52440601c7a0bbcf8](https://github.com/bitcoin/bitcoin/commit/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8) before running it with `-prune`.
@andrewtoth is this what you expected to happen, or was I measuring something wrong here?
<details>
<summary>Details</summary>
```bash
# git rev-parse HEAD
6d7f24595b08b8d1eba53e648533bcf87c30b48f
# git log --grep="bitcoin/bitcoin#28280"
commit
...
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2368014082)
> Didn’t #28280 resolve this?
That's why I was checking it, I've rebased it on [9cb9651d92ddb5d92724f6a52440601c7a0bbcf8](https://github.com/bitcoin/bitcoin/commit/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8) before running it with `-prune`.
@andrewtoth is this what you expected to happen, or was I measuring something wrong here?
<details>
<summary>Details</summary>
```bash
# git rev-parse HEAD
6d7f24595b08b8d1eba53e648533bcf87c30b48f
# git log --grep="bitcoin/bitcoin#28280"
commit
...
📝 stratospher opened a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951)
resolves #29618.
This PR adds a config flag option `-v2onlyclearnet` which disallows v1 connections on ipv4 and ipv6 networks since they're unencrypted. v1 connections are still possible from tor/i2p/cjdns peers since they're encrypted networks.
- v1 outbound connections to peers are not attempted
- v1 reconnections are not attempted (if peer is falsely advertised as v2 peer and we attempt a v2 connection but it fails, we do not attempt a v1 reconnection when `-v2onlyclearnet` is switched
...
(https://github.com/bitcoin/bitcoin/pull/30951)
resolves #29618.
This PR adds a config flag option `-v2onlyclearnet` which disallows v1 connections on ipv4 and ipv6 networks since they're unencrypted. v1 connections are still possible from tor/i2p/cjdns peers since they're encrypted networks.
- v1 outbound connections to peers are not attempted
- v1 reconnections are not attempted (if peer is falsely advertised as v2 peer and we attempt a v2 connection but it fails, we do not attempt a v1 reconnection when `-v2onlyclearnet` is switched
...
💬 stratospher commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2368065103)
opened a possible solution in https://github.com/bitcoin/bitcoin/pull/30951
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2368065103)
opened a possible solution in https://github.com/bitcoin/bitcoin/pull/30951
💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368086019)
> > Dropped the first refactoring commit
>
> Do you think that was the cause of the indeterminism, or are you still investigating that? (might be a followup)
Another commit with a fix for indeterminism has been pushed.
The PR description has been updated accordingly.
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368086019)
> > Dropped the first refactoring commit
>
> Do you think that was the cause of the indeterminism, or are you still investigating that? (might be a followup)
Another commit with a fix for indeterminism has been pushed.
The PR description has been updated accordingly.
👋 stratospher's pull request is ready for review: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988)
(https://github.com/bitcoin/bitcoin/pull/26988)
🤔 furszy reviewed a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2321929633)
Code review ACK
Commit 19bc71caf6a43702946f1bc15696fe14afa24fda description says "delay" when it should say "check_interval".
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2321929633)
Code review ACK
Commit 19bc71caf6a43702946f1bc15696fe14afa24fda description says "delay" when it should say "check_interval".
💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771246533)
nit:
`PEP 8: E302 expected 2 blank lines, found 1`
Should add another blank line above and below this function.
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771246533)
nit:
`PEP 8: E302 expected 2 blank lines, found 1`
Should add another blank line above and below this function.
💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771251127)
ultra tiny nit: same code, a bit shorter.
```python3
predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
while True:
if not predicate():
raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
if time.time() > time_end:
return
time.sleep(check_interval)
```
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771251127)
ultra tiny nit: same code, a bit shorter.
```python3
predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
while True:
if not predicate():
raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
if time.time() > time_end:
return
time.sleep(check_interval)
```
💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-2368089819)
Rebased. thanks @jonatack! reaching out.
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-2368089819)
Rebased. thanks @jonatack! reaching out.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2368093192)
@glozow @instagibbs It occurs to me that with the change in this PR, the `Cluster` will really only be used in tests anymore, as `DepGraph` objects can and will be built incrementally using `AddTransaction`, `AddDependencies`, and `RemoveTransactions`.
Because of that, I think it would be simpler to get rid of `Cluster` entirely, and reformulate the tests to all operate on `DepGraph` directly. I can do this in this PR, or as a follow-up. What do you think?
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2368093192)
@glozow @instagibbs It occurs to me that with the change in this PR, the `Cluster` will really only be used in tests anymore, as `DepGraph` objects can and will be built incrementally using `AddTransaction`, `AddDependencies`, and `RemoveTransactions`.
Because of that, I think it would be simpler to get rid of `Cluster` entirely, and reformulate the tests to all operate on `DepGraph` directly. I can do this in this PR, or as a follow-up. What do you think?
🤔 hodlinator reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013)
`git range-diff master 1e37bcf 41bdf3d`
Adds code disallowing combining `ALLOW_BOOL` with `ALLOW_INT` and `ALLOW_STRING`. Removes now invalid tests and adds 2 `BOOST_CHECK_THROW()`s.
Curious to hear maflcko's thoughts now that this change was made (a month ago). Happy to see if I can shake some stuff out first through.
## Making the applying of arguments testable
hodlinator:
> > If the C++ code were strictly organized with pairs of free functions with the first calling AddArg() an
...
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013)
`git range-diff master 1e37bcf 41bdf3d`
Adds code disallowing combining `ALLOW_BOOL` with `ALLOW_INT` and `ALLOW_STRING`. Removes now invalid tests and adds 2 `BOOST_CHECK_THROW()`s.
Curious to hear maflcko's thoughts now that this change was made (a month ago). Happy to see if I can shake some stuff out first through.
## Making the applying of arguments testable
hodlinator:
> > If the C++ code were strictly organized with pairs of free functions with the first calling AddArg() an
...
💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1767576075)
nit: Remove double spaces
```suggestion
throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_BOOL flag is incompatible with DISALLOW_ELISION "
"(boolean arguments should not require argument values)", arg_name));
}
if ((flags & ALLOW_INT) && (flags & ALLOW_STRING)) {
throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_INT flag is incompatible with ALLOW_STRING "
"(any val
...
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1767576075)
nit: Remove double spaces
```suggestion
throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_BOOL flag is incompatible with DISALLOW_ELISION "
"(boolean arguments should not require argument values)", arg_name));
}
if ((flags & ALLOW_INT) && (flags & ALLOW_STRING)) {
throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_INT flag is incompatible with ALLOW_STRING "
"(any val
...
💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771310361)
Feels like this is encouraging `BOOL | STRING` usage and should be broken out too if we are postponing that support.
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771310361)
Feels like this is encouraging `BOOL | STRING` usage and should be broken out too if we are postponing that support.
💬 willcl-ark commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2368164951)
> > set `CCACHE_COMPILERCHECK` to `content` mode, which passes the compiler check when the hash of the compiler is the same (the default is `mdate` which will change with each container we create)
>
> Are you sure? At least on Ubuntu Noble `mdate` is set correctly for `clang` (and does not change with each container). I presume the same is true for Debian and GCC. So, if the `modify` is different, the content would be different as well (and vice-versa). See:
>
> ```
> # stat /usr/bin/clan
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2368164951)
> > set `CCACHE_COMPILERCHECK` to `content` mode, which passes the compiler check when the hash of the compiler is the same (the default is `mdate` which will change with each container we create)
>
> Are you sure? At least on Ubuntu Noble `mdate` is set correctly for `clang` (and does not change with each container). I presume the same is true for Debian and GCC. So, if the `modify` is different, the content would be different as well (and vice-versa). See:
>
> ```
> # stat /usr/bin/clan
...
💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771467223)
> I think your advice to split up this PR is basically the same advice you are giving in #25665 to split up that PR, and my resistance in both PRs has been counterproductive.
It is certainly useful to have a working end-result that reviewers can look at, than not to have one. However, given an end-result, splitting up stuff for review (where it makes sense on a case-by-case basis) should have no downside, because reviewers can confirm that they agree with the overall goal and that the stuff i
...
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771467223)
> I think your advice to split up this PR is basically the same advice you are giving in #25665 to split up that PR, and my resistance in both PRs has been counterproductive.
It is certainly useful to have a working end-result that reviewers can look at, than not to have one. However, given an end-result, splitting up stuff for review (where it makes sense on a case-by-case basis) should have no downside, because reviewers can confirm that they agree with the overall goal and that the stuff i
...
💬 andrewtoth commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2368303170)
I really think there's too much variance when syncing with untrusted peers.
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2368303170)
I really think there's too much variance when syncing with untrusted peers.
💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2368341651)
> Curious to hear maflcko's thoughts now that this change was made (a month ago). Happy to see if I can shake some stuff out first through.
This needs rebase and is still in draft, which is why I haven't taken another look for now. Though, I may take another look later.
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2368341651)
> Curious to hear maflcko's thoughts now that this change was made (a month ago). Happy to see if I can shake some stuff out first through.
This needs rebase and is still in draft, which is why I haven't taken another look for now. Though, I may take another look later.
👍 ryanofsky approved a pull request: "depends: Fix build with `MULTIPROCESS=1` in Guix environment"
(https://github.com/bitcoin/bitcoin/pull/30940#pullrequestreview-2322355196)
Code review ACK 06b4c339e89e593d951a90cd2d1bce944acf3bf7
>> This causes CMake to search for package configurations in the native subdirectory first.
>
> This is the correct behaviour as far as we are concerned right?
I think the problem here is not that that cmake is searching the native directory first, but that it is searching the native directory only for the libmultiprocess runtime library, which is cross compiled, and can't be found in the native prefix.
The fix in d8e3afc3352a27
...
(https://github.com/bitcoin/bitcoin/pull/30940#pullrequestreview-2322355196)
Code review ACK 06b4c339e89e593d951a90cd2d1bce944acf3bf7
>> This causes CMake to search for package configurations in the native subdirectory first.
>
> This is the correct behaviour as far as we are concerned right?
I think the problem here is not that that cmake is searching the native directory first, but that it is searching the native directory only for the libmultiprocess runtime library, which is cross compiled, and can't be found in the native prefix.
The fix in d8e3afc3352a27
...