💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202202526)
> I don't think it's helpful to be so dismissive. Shipping new binaries (but not on all platforms), recommending new ways of running Core, and deprecating old ones shouldn't be taken lightly.
I don't think I am being dismissive, but sorry if anything came across that way. I understand there have been a lot of PRs to keep track of and the interactions between them may be confusing, but I feel if anything this approach is more conservative than what sipa is suggesting, and is compatible with hi
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202202526)
> I don't think it's helpful to be so dismissive. Shipping new binaries (but not on all platforms), recommending new ways of running Core, and deprecating old ones shouldn't be taken lightly.
I don't think I am being dismissive, but sorry if anything came across that way. I understand there have been a lot of PRs to keep track of and the interactions between them may be confusing, but I feel if anything this approach is more conservative than what sipa is suggesting, and is compatible with hi
...
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202213363)
> You and I know that, yes, but figuring out what's changed may be non-trivial for a user.
Is it possible to state more concretely what may be confusing here? The new `bitcoin` binary is already enabled in #31375. The only thing this PR affects is the behavior of the -m option, if specified, and the contents of the libexec directory.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202213363)
> You and I know that, yes, but figuring out what's changed may be non-trivial for a user.
Is it possible to state more concretely what may be confusing here? The new `bitcoin` binary is already enabled in #31375. The only thing this PR affects is the behavior of the -m option, if specified, and the contents of the libexec directory.
💬 theuni commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202236270)
> > I don't think it's helpful to be so dismissive. Shipping new binaries (but not on all platforms), recommending new ways of running Core, and deprecating old ones shouldn't be taken lightly.
>
> I don't think I am being dismissive, but sorry if anything came across that way. I understand there have been a lot of PRs to keep track of and the interactions between them may be confusing, but I feel if anything this approach is more conservative than what sipa is suggesting, and is compatible w
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202236270)
> > I don't think it's helpful to be so dismissive. Shipping new binaries (but not on all platforms), recommending new ways of running Core, and deprecating old ones shouldn't be taken lightly.
>
> I don't think I am being dismissive, but sorry if anything came across that way. I understand there have been a lot of PRs to keep track of and the interactions between them may be confusing, but I feel if anything this approach is more conservative than what sipa is suggesting, and is compatible w
...
💬 sipa commented on pull request "miner: clamp options instead of asserting":
(https://github.com/bitcoin/bitcoin/pull/33222#issuecomment-3202248904)
This makes sense for v30.0, I think, if we're going to be shipping mining interface support (#31802 or variation thereof).
(https://github.com/bitcoin/bitcoin/pull/33222#issuecomment-3202248904)
This makes sense for v30.0, I think, if we're going to be shipping mining interface support (#31802 or variation thereof).
💬 polespinasa commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3202259011)
> If latency is not very important, there may be a simpler solution: compute the template, and schedule it for sending, but only send it as soon as all transactions in it have been announced.
Maybe a stupid question and I'm not understanding something, but what is the point of this if we have to wait for all transactions to be announced? Isn't the whole idea to make sure our peers know about what we think will be the next block?
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3202259011)
> If latency is not very important, there may be a simpler solution: compute the template, and schedule it for sending, but only send it as soon as all transactions in it have been announced.
Maybe a stupid question and I'm not understanding something, but what is the point of this if we have to wait for all transactions to be announced? Isn't the whole idea to make sure our peers know about what we think will be the next block?
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286361853)
It really feels like the default constructor for this should be deleted, but that's not possible because `CChainParams()` default constructs it: https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/kernel/chainparams.h#L165
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286361853)
It really feels like the default constructor for this should be deleted, but that's not possible because `CChainParams()` default constructs it: https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/kernel/chainparams.h#L165
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286363848)
why limit these to `* 2`?
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286363848)
why limit these to `* 2`?
🤔 murchandamus reviewed a pull request: "wallet: Remove isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-3133501440)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-3133501440)
Concept ACK
💬 murchandamus commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2286095170)
In "wallet: Remove COutput::spendable and AvailableCoinsListUnspent" (1f085fa90ae92d1fda437066f802c1fe9c1dfe1c), I was a bit surprised to find this change in the logic in the commit as it does not seem to be explicitly mentioned in the commit message.
Prior to this change, we would proceed on UTXOs that are `spendable` or on UTXOs that are solvable while keys are disabled. After this change, we proceed either if keys are enabled OR the UTXO is solvable.
If we consider all UTXOs in descript
...
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2286095170)
In "wallet: Remove COutput::spendable and AvailableCoinsListUnspent" (1f085fa90ae92d1fda437066f802c1fe9c1dfe1c), I was a bit surprised to find this change in the logic in the commit as it does not seem to be explicitly mentioned in the commit message.
Prior to this change, we would proceed on UTXOs that are `spendable` or on UTXOs that are solvable while keys are disabled. After this change, we proceed either if keys are enabled OR the UTXO is solvable.
If we consider all UTXOs in descript
...
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286384092)
Ah, I missed that at least one of the params gets checked for not being default-constructed here:
https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/headerssync.cpp#L20
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286384092)
Ah, I missed that at least one of the params gets checked for not being default-constructed here:
https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/headerssync.cpp#L20
💬 ryanofsky commented on issue "depends: `native_libmultiprocess` fails to build on OpenBSD":
(https://github.com/bitcoin/bitcoin/issues/33219#issuecomment-3202305628)
Thanks for the report and clear reproduction steps. Following change fixes the problem for me, and I guess this was caused by the IWYU commit in https://github.com/bitcoin-core/libmultiprocess/pull/184,
```
--- a/src/ipc/libmultiprocess/src/mp/util.cpp
+++ b/src/ipc/libmultiprocess/src/mp/util.cpp
@@ -16,6 +16,7 @@
#include <sys/socket.h>
#include <sys/wait.h>
#include <system_error>
+#include <thread>
#include <unistd.h>
#include <utility>
#include <vector>
```
An alternate change also
...
(https://github.com/bitcoin/bitcoin/issues/33219#issuecomment-3202305628)
Thanks for the report and clear reproduction steps. Following change fixes the problem for me, and I guess this was caused by the IWYU commit in https://github.com/bitcoin-core/libmultiprocess/pull/184,
```
--- a/src/ipc/libmultiprocess/src/mp/util.cpp
+++ b/src/ipc/libmultiprocess/src/mp/util.cpp
@@ -16,6 +16,7 @@
#include <sys/socket.h>
#include <sys/wait.h>
#include <system_error>
+#include <thread>
#include <unistd.h>
#include <utility>
#include <vector>
```
An alternate change also
...
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286406511)
nit, I think this was wrong in the original test as well, should be:
```suggestion
CHECK_RESULT(hss.ProcessNextHeaders(std::span{first_chain}.last(first_chain.size() - 1), false),
```
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286406511)
nit, I think this was wrong in the original test as well, should be:
```suggestion
CHECK_RESULT(hss.ProcessNextHeaders(std::span{first_chain}.last(first_chain.size() - 1), false),
```
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286407697)
nit: I think this comment was incorrectly placed in the original, it is the `ProcessNextHeaders` above that needs `/*full_headers_message=*/true`
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286407697)
nit: I think this comment was incorrectly placed in the original, it is the `ProcessNextHeaders` above that needs `/*full_headers_message=*/true`
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286413175)
nit:
```suggestion
CHECK_RESULT(hss.ProcessNextHeaders({first_chain.begin() + REDOWNLOAD_BUFFER_SIZE + 1, first_chain.end()}, false),
```
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286413175)
nit:
```suggestion
CHECK_RESULT(hss.ProcessNextHeaders({first_chain.begin() + REDOWNLOAD_BUFFER_SIZE + 1, first_chain.end()}, false),
```
💬 achow101 commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2286418712)
Yes, I believe it is obsolete. Removed.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2286418712)
Yes, I believe it is obsolete. Removed.
💬 TheBlueMatt commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202364764)
> I don't see how it's required to use a fork of Core, unless you insist on running pre-built binaries only. This may be somewhat naive, but it doesn't seem that difficult to encourage users that want SV2 to use v30 with IPC enabled, and then open up issues as bugs turn up
- @marcofleon
Yes, this is somewhat naive. We've discussed this more than a few times and the reality is that people just don't build from source. We've been talking to miners for a long time and telling them "just take Sjors
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202364764)
> I don't see how it's required to use a fork of Core, unless you insist on running pre-built binaries only. This may be somewhat naive, but it doesn't seem that difficult to encourage users that want SV2 to use v30 with IPC enabled, and then open up issues as bugs turn up
- @marcofleon
Yes, this is somewhat naive. We've discussed this more than a few times and the reality is that people just don't build from source. We've been talking to miners for a long time and telling them "just take Sjors
...
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286387382)
Agreed, would be nice to have each chain's params be a `constexpr` expression tree in an ideal world.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286387382)
Agreed, would be nice to have each chain's params be a `constexpr` expression tree in an ideal world.
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286383684)
That might be a cool higher level change, although hopefully a commitment will mismatch before we reach the final header.
Hm... thinking through the current code - given 15'000 headers and a period of 600, we have 38 commitments. So there's a 1 in 2^38 chance that all commitments will match. There's a 1/600 chance that `HeadersSyncState::commit_offset` will be 599, in which case `second_chain` would be too short to check the last commitment, making it a 1 in 2^37 chance of test failure for th
...
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286383684)
That might be a cool higher level change, although hopefully a commitment will mismatch before we reach the final header.
Hm... thinking through the current code - given 15'000 headers and a period of 600, we have 38 commitments. So there's a 1 in 2^38 chance that all commitments will match. There's a 1/600 chance that `HeadersSyncState::commit_offset` will be 599, in which case `second_chain` would be too short to check the last commitment, making it a 1 in 2^37 chance of test failure for th
...
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286389214)
Giving us 2x mainnet should give the fuzzers enough time to break things before we reach that level. See 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b for an example of how gradually these values change for 1 release.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286389214)
Giving us 2x mainnet should give the fuzzers enough time to break things before we reach that level. See 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b for an example of how gradually these values change for 1 release.
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286437807)
Agree here, it seems to be a more taxing test to not pretend like there are any more headers, that `HeadersSyncState` will have to make do. Will consider changing this if I push, although it is the same on master. Maybe making it random or doing a `for x in {true, false}` loop over it would be even better.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286437807)
Agree here, it seems to be a more taxing test to not pretend like there are any more headers, that `HeadersSyncState` will have to make do. Will consider changing this if I push, although it is the same on master. Maybe making it random or doing a `for x in {true, false}` loop over it would be even better.
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286427414)
Seems related to your previous GH-comment. Current PR version has the comment:
https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/test/headers_sync_chainwork_tests.cpp#L153
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286427414)
Seems related to your previous GH-comment. Current PR version has the comment:
https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/test/headers_sync_chainwork_tests.cpp#L153