💬 0xB10C commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#discussion_r1602136436)
I agree that this would work. I'm a bit reluctant to introduce more log parsing for tests. I'm leaving unresolved: If someone feels strongly about doing this, let me know.
An alternative I have though about is adding the required version information to `test/config.ini`. Again, also open to do this if someone thinks it's important to do it.
(https://github.com/bitcoin/bitcoin/pull/30112#discussion_r1602136436)
I agree that this would work. I'm a bit reluctant to introduce more log parsing for tests. I'm leaving unresolved: If someone feels strongly about doing this, let me know.
An alternative I have though about is adding the required version information to `test/config.ini`. Again, also open to do this if someone thinks it's important to do it.
💬 0xB10C commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2113298531)
> > The windows failure is:
> > ```
> > D:\a\bitcoin\bitcoin\src\rpc\node.cpp(223,45): error C2065: 'CLIENT_VERSION_RC': undeclared identifier [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]
> > ```
>
> Should fixed with:
>
> ```diff
> --- a/build_msvc/bitcoin_config.h.in
> +++ b/build_msvc/bitcoin_config.h.in
> @@ -11,6 +11,9 @@
> /* Version is release */
> #define CLIENT_VERSION_IS_RELEASE $
>
> +/* Release candidate */
> +#define CLIENT_VERSION_R
...
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2113298531)
> > The windows failure is:
> > ```
> > D:\a\bitcoin\bitcoin\src\rpc\node.cpp(223,45): error C2065: 'CLIENT_VERSION_RC': undeclared identifier [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]
> > ```
>
> Should fixed with:
>
> ```diff
> --- a/build_msvc/bitcoin_config.h.in
> +++ b/build_msvc/bitcoin_config.h.in
> @@ -11,6 +11,9 @@
> /* Version is release */
> #define CLIENT_VERSION_IS_RELEASE $
>
> +/* Release candidate */
> +#define CLIENT_VERSION_R
...
💬 0xB10C commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2113308561)
> Should we eventually deprecate `version` and `subversion` fields from `getnetworkinfo`? If so, updating the `getnetworkinfo` docs is probably in order? No need to rush deprecating anything just yet, but just to indicate the direction?
I'm not too sure about removing `subversion`, but I think we can deprecate `version` at some point as it's identical to `numeric` in `getversion`. If I understand you correctly, you mean adding a note to the `version` docs that users should use `getversion` go
...
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2113308561)
> Should we eventually deprecate `version` and `subversion` fields from `getnetworkinfo`? If so, updating the `getnetworkinfo` docs is probably in order? No need to rush deprecating anything just yet, but just to indicate the direction?
I'm not too sure about removing `subversion`, but I think we can deprecate `version` at some point as it's identical to `numeric` in `getversion`. If I understand you correctly, you mean adding a note to the `version` docs that users should use `getversion` go
...
💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#discussion_r1602153175)
Sorry, that's wrong, I meant to put them just on the private section. I'll fix it soon. Thanks!
(https://github.com/bitcoin-core/gui/pull/762#discussion_r1602153175)
Sorry, that's wrong, I meant to put them just on the private section. I'll fix it soon. Thanks!
💬 pinheadmz commented on pull request "p2p: detect addnode cjdns peers in GetAddedNodeInfo()":
(https://github.com/bitcoin/bitcoin/pull/30085#issuecomment-2113332863)
Confirmed this branch fixes the issue in production on mainnet as well:
```
$ bccli getaddednodeinfo
[
{
"addednode": "fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa",
"connected": false,
"addresses": [
]
},
{
"addednode": "fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce",
"connected": true,
"addresses": [
{
"address": "[fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce]:8333",
"connected": "outbound"
}
]
}
]
```
(https://github.com/bitcoin/bitcoin/pull/30085#issuecomment-2113332863)
Confirmed this branch fixes the issue in production on mainnet as well:
```
$ bccli getaddednodeinfo
[
{
"addednode": "fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa",
"connected": false,
"addresses": [
]
},
{
"addednode": "fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce",
"connected": true,
"addresses": [
{
"address": "[fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce]:8333",
"connected": "outbound"
}
]
}
]
```
💬 jonatack commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2113340979)
Concept ACK, would need a release note.
> I'm not too sure about removing `subversion`, but I think we can deprecate `version`
Note that removing `version` from getnetworkinfo would break some bitcoin-cli calls to long-running nodes, unless handled with a version check and would also add an additional RPC call there. Probably not worth it to remove anything.
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2113340979)
Concept ACK, would need a release note.
> I'm not too sure about removing `subversion`, but I think we can deprecate `version`
Note that removing `version` from getnetworkinfo would break some bitcoin-cli calls to long-running nodes, unless handled with a version check and would also add an additional RPC call there. Probably not worth it to remove anything.
✅ willcl-ark closed an issue: "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt"
(https://github.com/bitcoin/bitcoin/issues/30020)
(https://github.com/bitcoin/bitcoin/issues/30020)
💬 willcl-ark commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2113357989)
I'm going to close this for now.
I have tried to reproduce this in a fresh debian bookworm container (not a raspi) but was unable to do so.
I configured, compiled and made with `--with-gui=no`, both with and without the gui dependencies installed (in case that made any difference), but in both cases the `./src/qt/test/test_bitcoin-qt` binary was not built, and so could not be tested (and fail the test as reported here).
If you could provide specific steps for reproduction (leave a comme
...
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2113357989)
I'm going to close this for now.
I have tried to reproduce this in a fresh debian bookworm container (not a raspi) but was unable to do so.
I configured, compiled and made with `--with-gui=no`, both with and without the gui dependencies installed (in case that made any difference), but in both cases the `./src/qt/test/test_bitcoin-qt` binary was not built, and so could not be tested (and fail the test as reported here).
If you could provide specific steps for reproduction (leave a comme
...
📝 sr-gi opened a pull request: "p2p: Fill reconciliation sets (Erlay) attempt: 2"
(https://github.com/bitcoin/bitcoin/pull/30116)
This is a re-attempt of https://github.com/bitcoin/bitcoin/pull/28765
The main differences from it are:
- Most outstanding comments have been addressed (or responded to on the original PR)
- The description of how a node is picked in `IsFanoutTarget` has been updated to reflect what the algorithm is doing (not how it is doing it).
- The way `hash_key` is seeded in `IsFanoutTarget` has changed (from `m_k0` to `wtxid.ToUint256()`). This is to prevent using `m_k0` for something it is not in
...
(https://github.com/bitcoin/bitcoin/pull/30116)
This is a re-attempt of https://github.com/bitcoin/bitcoin/pull/28765
The main differences from it are:
- Most outstanding comments have been addressed (or responded to on the original PR)
- The description of how a node is picked in `IsFanoutTarget` has been updated to reflect what the algorithm is doing (not how it is doing it).
- The way `hash_key` is seeded in `IsFanoutTarget` has changed (from `m_k0` to `wtxid.ToUint256()`). This is to prevent using `m_k0` for something it is not in
...
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt: 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2113365602)
I've talked to @naumenkogs about picking this up and he was happy about it. I'm happy to close this if he changes his mind.
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2113365602)
I've talked to @naumenkogs about picking this up and he was happy about it. I'm happy to close this if he changes his mind.
💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1602211367)
> I need to test this, but this would be surprising to me.
Indeed, my impression was wrong. When we restart after a previous reindex without passing in `-reindex`, the first step is activating the chain again.
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1602211367)
> I need to test this, but this would be surprising to me.
Indeed, my impression was wrong. When we restart after a previous reindex without passing in `-reindex`, the first step is activating the chain again.
📝 Mmgg002 opened a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/30117)
<!--
*** 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 improve coverage are a
...
(https://github.com/bitcoin/bitcoin/pull/30117)
<!--
*** 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 improve coverage are a
...
👍 ryanofsky approved a pull request: "rpc: avoid copying into UniValue"
(https://github.com/bitcoin/bitcoin/pull/30115#pullrequestreview-2058984290)
Code review ACK 35b5b0ff9e043c1e26a3e29fa3d9d914a0a86ad1. I confirmed this change is only adding std::moves, and also tried to look for cases where an object was being used after a move, since that seems like the most likely type of bug this PR could introduce. I didn't check exhaustively, though, since that would be a lot more work and I think we have a clang-tidy check that would catch those cases.
(https://github.com/bitcoin/bitcoin/pull/30115#pullrequestreview-2058984290)
Code review ACK 35b5b0ff9e043c1e26a3e29fa3d9d914a0a86ad1. I confirmed this change is only adding std::moves, and also tried to look for cases where an object was being used after a move, since that seems like the most likely type of bug this PR could introduce. I didn't check exhaustively, though, since that would be a lot more work and I think we have a clang-tidy check that would catch those cases.
📝 furszy opened a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118)
Decoupled from #27837 because this can help other too, found it investigating a CI failure https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200.
The `connect_nodes` function in the test framework relies on a stable number of peer
connections to verify the new connection between the nodes is successfully established.
This approach is fragile, as any of the peers involved in the process can drop, lose, or
create a connection at any step, causing subsequent `wait_until` checks to stall i
...
(https://github.com/bitcoin/bitcoin/pull/30118)
Decoupled from #27837 because this can help other too, found it investigating a CI failure https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200.
The `connect_nodes` function in the test framework relies on a stable number of peer
connections to verify the new connection between the nodes is successfully established.
This approach is fragile, as any of the peers involved in the process can drop, lose, or
create a connection at any step, causing subsequent `wait_until` checks to stall i
...
✅ hebasto closed a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/30117)
(https://github.com/bitcoin/bitcoin/pull/30117)
📝 hebasto locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/30117)
<!--
*** 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 improve coverage are a
...
(https://github.com/bitcoin/bitcoin/pull/30117)
<!--
*** 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 improve coverage are a
...
💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1602301949)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" 7c047d30cb0eadc8a424cb01de2fcd0978e22206: What is the step 2? This comment seems a little confuse because I couldn't find it as I see for Step 1.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1602301949)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" 7c047d30cb0eadc8a424cb01de2fcd0978e22206: What is the step 2? This comment seems a little confuse because I couldn't find it as I see for Step 1.
💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1602304001)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" https://github.com/bitcoin/bitcoin/commit/7c047d30cb0eadc8a424cb01de2fcd0978e22206: Perhaps adding a log when removing from set?
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1602304001)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" https://github.com/bitcoin/bitcoin/commit/7c047d30cb0eadc8a424cb01de2fcd0978e22206: Perhaps adding a log when removing from set?
💬 kevkevinpal commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1602311815)
Thanks for the review!
I think the timeout should be fine there is even a comment pointing out that we check the work twice because we want to fail fast
https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5655-L5657
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1602311815)
Thanks for the review!
I think the timeout should be fine there is even a comment pointing out that we check the work twice because we want to fail fast
https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5655-L5657
💬 1ma commented on issue "contrib/signet/miner: grind will fail for high difficulty chain":
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2113592794)
Can reproduce. Been running a signet network whose miner stalled a few dozens of blocks after the block 10080 difficulty adjustment.
The miner is ran as a systemd service like this, and the nbits parameter was determined by using the calibrating utility set to 600s. Despite this, before block 10080 the miner used to mine a block every 2.5 min instead of 10. I don't quite understand how it works, honestly.
```shell
miner --cli="bitcoin-cli" generate --address $rewards_addy --grind-cmd="bit
...
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2113592794)
Can reproduce. Been running a signet network whose miner stalled a few dozens of blocks after the block 10080 difficulty adjustment.
The miner is ran as a systemd service like this, and the nbits parameter was determined by using the calibrating utility set to 600s. Despite this, before block 10080 the miner used to mine a block every 2.5 min instead of 10. I don't quite understand how it works, honestly.
```shell
miner --cli="bitcoin-cli" generate --address $rewards_addy --grind-cmd="bit
...