💬 MarcoFalke commented on pull request "ci: Add missing libclang-rt-dev package":
(https://github.com/bitcoin/bitcoin/pull/27323#issuecomment-1482831967)
Ah sorry. This requires bumping the image at the same time. Closing.
(https://github.com/bitcoin/bitcoin/pull/27323#issuecomment-1482831967)
Ah sorry. This requires bumping the image at the same time. Closing.
✅ MarcoFalke closed a pull request: "ci: Add missing libclang-rt-dev package"
(https://github.com/bitcoin/bitcoin/pull/27323)
(https://github.com/bitcoin/bitcoin/pull/27323)
💬 jnewbery commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1147619018)
> Alas that does not work if the condition includes function calls.
Why not? From the GDB docs:
> Break conditions can have side effects, and may even call functions in your program.
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1147619018)
> Alas that does not work if the condition includes function calls.
Why not? From the GDB docs:
> Break conditions can have side effects, and may even call functions in your program.
💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1147633787)
Just a suggestion to make the code a lit bit cleaner, perhaps we don't need to create `position` and `nId` again after the loop.
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index cdfd079fc..30ce2cadc 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -757,10 +757,10 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
// Iterate over the positions of that bucket, starting at the initial one,
// and looping around.
-
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1147633787)
Just a suggestion to make the code a lit bit cleaner, perhaps we don't need to create `position` and `nId` again after the loop.
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index cdfd079fc..30ce2cadc 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -757,10 +757,10 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
// Iterate over the positions of that bucket, starting at the initial one,
// and looping around.
-
...
💬 MarcoFalke commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1482879003)
> Ran into a failure (with the above branch, and NO_BDB=1), on aarch64:
Does this happen on master as well?
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1482879003)
> Ran into a failure (with the above branch, and NO_BDB=1), on aarch64:
Does this happen on master as well?
💬 MarcoFalke commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1482882959)
See also https://github.com/bitcoin/bitcoin/pull/26188
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1482882959)
See also https://github.com/bitcoin/bitcoin/pull/26188
💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1147671339)
About the optimization in https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132629513, couldn't we simply use a vector to store the visited buckets?
just an example:
```cpp
std::vector<int> buckets_visited;
while (1) {
// Pick a bucket, and an initial position in that bucket.
int bucket = insecure_rand.randrange(bucket_count);
if (std::find(buckets_visited.begin(), buckets_visited.end(), bucket) != buckets_visited.end()) {
continue;
} els
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1147671339)
About the optimization in https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132629513, couldn't we simply use a vector to store the visited buckets?
just an example:
```cpp
std::vector<int> buckets_visited;
while (1) {
// Pick a bucket, and an initial position in that bucket.
int bucket = insecure_rand.randrange(bucket_count);
if (std::find(buckets_visited.begin(), buckets_visited.end(), bucket) != buckets_visited.end()) {
continue;
} els
...
💬 jonatack commented on pull request "move-only: IsDeprecatedRPCEnabled to rpc/util to fix CI builds when called from wallet":
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482910548)
PR description and commit message updated per https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482807752 (thanks @MarcoFalke).
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482910548)
PR description and commit message updated per https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482807752 (thanks @MarcoFalke).
💬 sipa commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147676124)
I don't think there is a need for these `is_rfc8439` values, because overflowing the block counter in RFC8439 mode is simply not allowed. If it happens, both incrementing j13 or not incrementing it are wrong. If you want to do anything, it should be an assertion failure.
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147676124)
I don't think there is a need for these `is_rfc8439` values, because overflowing the block counter in RFC8439 mode is simply not allowed. If it happens, both incrementing j13 or not incrementing it are wrong. If you want to do anything, it should be an assertion failure.
💬 sipa commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147677294)
I think this variable can be constant (and if it isn't, it probably shouldn't be a global). Also, globals should be upper case.
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147677294)
I think this variable can be constant (and if it isn't, it probably shouldn't be a global). Also, globals should be upper case.
💬 sipa commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147678589)
`static const` here as well?
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147678589)
`static const` here as well?
💬 ryanofsky commented on pull request "system: allow GUI to initialize default data dir":
(https://github.com/bitcoin/bitcoin/pull/27273#discussion_r1147632734)
In commit "system: allow GUI to initialize default data dir" (7a9b786e00a5275ba012bc37a3796d1d35491161)
This `if` statement no longer does anything and could be dropped. Just calling `InitDefaultDataDir` unconditionally would have the same effect and be easier to understand. Also the comment above needs to be updated. Could replace "Only override -datadir if different from the default, to make it possible [...]" with "Set chosen directory as the default datadir. It is possible [...]"
(https://github.com/bitcoin/bitcoin/pull/27273#discussion_r1147632734)
In commit "system: allow GUI to initialize default data dir" (7a9b786e00a5275ba012bc37a3796d1d35491161)
This `if` statement no longer does anything and could be dropped. Just calling `InitDefaultDataDir` unconditionally would have the same effect and be easier to understand. Also the comment above needs to be updated. Could replace "Only override -datadir if different from the default, to make it possible [...]" with "Set chosen directory as the default datadir. It is possible [...]"
💬 ryanofsky commented on pull request "system: allow GUI to initialize default data dir":
(https://github.com/bitcoin/bitcoin/pull/27273#discussion_r1147642262)
In commit "system: allow GUI to initialize default data dir" (7a9b786e00a5275ba012bc37a3796d1d35491161)
This `if` statment is unnecessary and causes `InitDefaultDataDir` to do nothing if it is called a second time. Would be saner to rename `InitDefaultDataDir` `SetDefaultDataDir` and just set the path unconditionally without unnecessary stickiness and no way of knowing whether the path was set.
(https://github.com/bitcoin/bitcoin/pull/27273#discussion_r1147642262)
In commit "system: allow GUI to initialize default data dir" (7a9b786e00a5275ba012bc37a3796d1d35491161)
This `if` statment is unnecessary and causes `InitDefaultDataDir` to do nothing if it is called a second time. Would be saner to rename `InitDefaultDataDir` `SetDefaultDataDir` and just set the path unconditionally without unnecessary stickiness and no way of knowing whether the path was set.
📝 dergoegge opened a pull request: "net: #27257 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/27324)
Follow-up PR for #27257
* Deletes the copy constructor/assignment operator of `CNetMessage`
* Removes trivial getter for the connection type
* Avoids passing `nRecvFloodSize` to CNode methods by passing it to `CNode` on creation
(https://github.com/bitcoin/bitcoin/pull/27324)
Follow-up PR for #27257
* Deletes the copy constructor/assignment operator of `CNetMessage`
* Removes trivial getter for the connection type
* Avoids passing `nRecvFloodSize` to CNode methods by passing it to `CNode` on creation
💬 mzumsande commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147725684)
> how important are IsTerrible addresses, and would it be interesting to have the breakdown with/without that filtering.
`IsTerrible` is used for two things outside of RPCs:
1.) Terrible addresses are not included in answers to GETADDR requests from peers
2.) In case of a conflict (two addrs would go to the same bucket and position in the new tables) the old addr is only overwritten if it's terrible.
Notably, `IsTerrible` isn't used as a filter while making outbound connections, which is
...
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147725684)
> how important are IsTerrible addresses, and would it be interesting to have the breakdown with/without that filtering.
`IsTerrible` is used for two things outside of RPCs:
1.) Terrible addresses are not included in answers to GETADDR requests from peers
2.) In case of a conflict (two addrs would go to the same bucket and position in the new tables) the old addr is only overwritten if it's terrible.
Notably, `IsTerrible` isn't used as a filter while making outbound connections, which is
...
💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147719940)
Is it possible for you to cache the config file path at the system level like I did [here](https://github.com/bitcoin/bitcoin/pull/27303/files#diff-2babd91c181a89aaeb4a2bd610c8b6bcab0f0b4f5dd20251a3ad0e1742b56380)? That would also fix the confusing log statement produced by the same datadir/conf configuration.
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147719940)
Is it possible for you to cache the config file path at the system level like I did [here](https://github.com/bitcoin/bitcoin/pull/27303/files#diff-2babd91c181a89aaeb4a2bd610c8b6bcab0f0b4f5dd20251a3ad0e1742b56380)? That would also fix the confusing log statement produced by the same datadir/conf configuration.
💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147740130)
Why do you need this os-specific path generator instead of just adding a single subdir to the direcotry where the test framework is already working? On my system at least, it doesn't actually return the "real" default path:
`/tmp/bitcoin_func_test_yg47qoyn/home/Library/Application Support/Bitcoin`
so why not just add `home/` on every platform and use that?
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147740130)
Why do you need this os-specific path generator instead of just adding a single subdir to the direcotry where the test framework is already working? On my system at least, it doesn't actually return the "real" default path:
`/tmp/bitcoin_func_test_yg47qoyn/home/Library/Application Support/Bitcoin`
so why not just add `home/` on every platform and use that?
💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147716697)
Good thinking about the portable datdir use case, I think it is ok to allow a bypass. It's a bit funny calling it "warn...=1" though. I understand you mean "warn instead of throw an error" but I almost think something like `-ignoreignoredconf` or `-ignoreextraconf` makes more literal sense.
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147716697)
Good thinking about the portable datdir use case, I think it is ok to allow a bypass. It's a bit funny calling it "warn...=1" though. I understand you mean "warn instead of throw an error" but I almost think something like `-ignoreignoredconf` or `-ignoreextraconf` makes more literal sense.
💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147745998)
clang-format wants this to be a single line but I prefer this style too, especially for these longer return values 🤷♂️
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147745998)
clang-format wants this to be a single line but I prefer this style too, especially for these longer return values 🤷♂️
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147763611)
Since we need to maintain compat with v22-v24 not to break user space, it seems helpful to return both so as not to disrupt signal/context as well for those who have been using -addrinfo (no need to break what wasn't broken), thus the idea in https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147473618. This would also allow more insight for users as to how much is filtered (and possibly to us regarding its usage in making connections).
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147763611)
Since we need to maintain compat with v22-v24 not to break user space, it seems helpful to return both so as not to disrupt signal/context as well for those who have been using -addrinfo (no need to break what wasn't broken), thus the idea in https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147473618. This would also allow more insight for users as to how much is filtered (and possibly to us regarding its usage in making connections).