🚀 fanquake merged a pull request: "Cluster mempool followups"
(https://github.com/bitcoin/bitcoin/pull/33591)
(https://github.com/bitcoin/bitcoin/pull/33591)
💬 fanquake commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3601252308)
@alfonsoromanz can you rebase/ followup with the test failures here?
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3601252308)
@alfonsoromanz can you rebase/ followup with the test failures here?
🚀 fanquake merged a pull request: "[30.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33609)
(https://github.com/bitcoin/bitcoin/pull/33609)
💬 ajtowns commented on issue "Net split meta issue":
(https://github.com/bitcoin/bitcoin/issues/33958#issuecomment-3601281266)
> Reduce `PeerManager`'s usage of `CNode`
> Move the message handling loop to `PeerManager`
Big fan of these aspects. I like the documentation aspect of NetManagerEvents. Would like to see the PeerManager functions move towards accepting a `Peer&` (vs a `CNode*` and looking up `GetPeerRef`, or an explicit `CNode, Peer` pair) in general too.
> Dropped usage of `CNode` in `PeerManager`
>
> With the event loop moved and nascent interfaces beginning to take shape, `PeerManager` should be able to
...
(https://github.com/bitcoin/bitcoin/issues/33958#issuecomment-3601281266)
> Reduce `PeerManager`'s usage of `CNode`
> Move the message handling loop to `PeerManager`
Big fan of these aspects. I like the documentation aspect of NetManagerEvents. Would like to see the PeerManager functions move towards accepting a `Peer&` (vs a `CNode*` and looking up `GetPeerRef`, or an explicit `CNode, Peer` pair) in general too.
> Dropped usage of `CNode` in `PeerManager`
>
> With the event loop moved and nascent interfaces beginning to take shape, `PeerManager` should be able to
...
💬 fanquake commented on pull request "depends, doc: Add `tcl` as build dependency for `sqlite` package":
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3601283102)
Can we just pass the compiler through?
(https://github.com/bitcoin/bitcoin/pull/33975#issuecomment-3601283102)
Can we just pass the compiler through?
💬 fanquake commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2580569951)
Should be in `compat.h`?
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2580569951)
Should be in `compat.h`?
💬 fanquake commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#issuecomment-3601316432)
cc @Sjors
(https://github.com/bitcoin/bitcoin/pull/33986#issuecomment-3601316432)
cc @Sjors
💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3601375713)
> if you think it would be useful for IPC clients to "have a way to signal their intent to use the node default"
sv2-tp always sets the value (based on `coinbase_output_max_additional_size` in `CoinbaseOutputConstraints`), so it wouldn't benefit. But it does seem better to allow clients to omit this value.
For that to work however, we'd also need to change mining.capnp, e.g.:
```capnp
blockReservedWeight @1 :UInt64 $Proxy.name("block_reserved_weight");
hasBlockReservedWeight @3 :Bool
...
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3601375713)
> if you think it would be useful for IPC clients to "have a way to signal their intent to use the node default"
sv2-tp always sets the value (based on `coinbase_output_max_additional_size` in `CoinbaseOutputConstraints`), so it wouldn't benefit. But it does seem better to allow clients to omit this value.
For that to work however, we'd also need to change mining.capnp, e.g.:
```capnp
blockReservedWeight @1 :UInt64 $Proxy.name("block_reserved_weight");
hasBlockReservedWeight @3 :Bool
...
💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2580623784)
The latest push leaves RPC code untouched.
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2580623784)
The latest push leaves RPC code untouched.
💬 willcl-ark commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3601387596)
> See [#33902 (comment)](https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641).
As I describe [here](https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2556778530) I have a commit which if `host_CC` is set, **and**** `CC` is unset, will set `CC` to `host_CC` (and similarly for `CXX` and friends).
In my opinion this would result in the expected behaviour:
- If `{var}` is set, use that
- If not check if `host_{var}` is set, and use that
- Otherwise, fallback to a
...
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3601387596)
> See [#33902 (comment)](https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641).
As I describe [here](https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2556778530) I have a commit which if `host_CC` is set, **and**** `CC` is unset, will set `CC` to `host_CC` (and similarly for `CXX` and friends).
In my opinion this would result in the expected behaviour:
- If `{var}` is set, use that
- If not check if `host_{var}` is set, and use that
- Otherwise, fallback to a
...
🤔 rkrux reviewed a pull request: "test: p2p: check that peer's announced starting height is remembered"
(https://github.com/bitcoin/bitcoin/pull/33990#pullrequestreview-3529561300)
Concept ~0 52f96cc235d309d9156eb742036c859984b9a467
I'm on the fence regarding whether this case testing just one field is warranted. I don't see much coverage gained in the `corecheck`.
Though there is a similar `test_desirable_service_flags` case as well but in it the connection is disconnected conditionally, so one can argue that there is more to test over there.
(https://github.com/bitcoin/bitcoin/pull/33990#pullrequestreview-3529561300)
Concept ~0 52f96cc235d309d9156eb742036c859984b9a467
I'm on the fence regarding whether this case testing just one field is warranted. I don't see much coverage gained in the `corecheck`.
Though there is a similar `test_desirable_service_flags` case as well but in it the connection is disconnected conditionally, so one can argue that there is more to test over there.
💬 brunoerg commented on pull request "test: p2p: check that peer's announced starting height is remembered":
(https://github.com/bitcoin/bitcoin/pull/33990#discussion_r2580745364)
nit: Perhaps a comment explaining these values (edge cases for 32-bit integers) would be good.
(https://github.com/bitcoin/bitcoin/pull/33990#discussion_r2580745364)
nit: Perhaps a comment explaining these values (edge cases for 32-bit integers) would be good.
💬 brunoerg commented on pull request "test: p2p: check that peer's announced starting height is remembered":
(https://github.com/bitcoin/bitcoin/pull/33990#issuecomment-3601527993)
Can be tested with the following diff:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 5dac413319..ec417b97ac 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3535,7 +3535,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
LOCK(pfrom.m_subver_mutex);
pfrom.cleanSubVer = cleanSubVer;
}
- peer->m_starting_height = starting_height;
+ peer->m_starting_height
...
(https://github.com/bitcoin/bitcoin/pull/33990#issuecomment-3601527993)
Can be tested with the following diff:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 5dac413319..ec417b97ac 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3535,7 +3535,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
LOCK(pfrom.m_subver_mutex);
pfrom.cleanSubVer = cleanSubVer;
}
- peer->m_starting_height = starting_height;
+ peer->m_starting_height
...
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3601633659)
> @alfonsoromanz can you rebase/ followup with the test failures here?
Yes, I will take care of it
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3601633659)
> @alfonsoromanz can you rebase/ followup with the test failures here?
Yes, I will take care of it
👍 laanwj approved a pull request: "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO"
(https://github.com/bitcoin/bitcoin/pull/33702#pullrequestreview-3529672889)
Re-ACK fad61185861a6a9ed806c387aa63d2b31262b1db
```
$ git range-diff 3bb30658e631ed45b6c8609292facc7ae3dd0f61..fa2fd0ba1fbd4085df24a2c5472636957db28521 b30262dcaa28c40a0a5072847b7194b3db203160..fad61185861a6a9ed806c387aa63d2b31262b1db
```
⋮
* `4: 4444edeecc528ccd88b7dd78ffddbccd69762132 ! 4: fae612424b3e70acd6011a4459518174463b3424 contrib: Remove confusing and redundant encoding from IO` - the only differing commit, checked that it is rebase
⋮
(https://github.com/bitcoin/bitcoin/pull/33702#pullrequestreview-3529672889)
Re-ACK fad61185861a6a9ed806c387aa63d2b31262b1db
```
$ git range-diff 3bb30658e631ed45b6c8609292facc7ae3dd0f61..fa2fd0ba1fbd4085df24a2c5472636957db28521 b30262dcaa28c40a0a5072847b7194b3db203160..fad61185861a6a9ed806c387aa63d2b31262b1db
```
⋮
* `4: 4444edeecc528ccd88b7dd78ffddbccd69762132 ! 4: fae612424b3e70acd6011a4459518174463b3424 contrib: Remove confusing and redundant encoding from IO` - the only differing commit, checked that it is rebase
⋮
💬 brunoerg commented on pull request "init: point out -stopatheight may be imprecise":
(https://github.com/bitcoin/bitcoin/pull/33993#issuecomment-3601640495)
Rebased
(https://github.com/bitcoin/bitcoin/pull/33993#issuecomment-3601640495)
Rebased
👍 stickies-v approved a pull request: "init: point out -stopatheight may be imprecise"
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3529707358)
ACK e4703863655135856d0d6ad54a7021d3326bf071
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3529707358)
ACK e4703863655135856d0d6ad54a7021d3326bf071
💬 stickies-v commented on pull request "init: point out -stopatheight may be imprecise":
(https://github.com/bitcoin/bitcoin/pull/33993#discussion_r2580857428)
"Blocks after target height may be processed during shutdown." is a bit more concise and has my preference.
(https://github.com/bitcoin/bitcoin/pull/33993#discussion_r2580857428)
"Blocks after target height may be processed during shutdown." is a bit more concise and has my preference.
⚠️ plebhash opened an issue: "Mining IPC `createNewBlock` should not return before IBD is over"
(https://github.com/bitcoin/bitcoin/issues/33994)
### Please describe the feature you'd like to see added.
Mining IPC `createNewBlock` should not return before IBD is over
### Is your feature related to a problem, if so please describe it.
on SRI [`bitcoin_core_sv2`](https://github.com/stratum-mining/sv2-apps/tree/main/bitcoin-core-sv2) crate, we want to wait for IBD is over as part of the bootstrapping processes
discussing with @ismaelsadeeq and @Shourya742 during BTrust dev day, we came to the conclusion that `createNewBlock` shouldn't ev
...
(https://github.com/bitcoin/bitcoin/issues/33994)
### Please describe the feature you'd like to see added.
Mining IPC `createNewBlock` should not return before IBD is over
### Is your feature related to a problem, if so please describe it.
on SRI [`bitcoin_core_sv2`](https://github.com/stratum-mining/sv2-apps/tree/main/bitcoin-core-sv2) crate, we want to wait for IBD is over as part of the bootstrapping processes
discussing with @ismaelsadeeq and @Shourya742 during BTrust dev day, we came to the conclusion that `createNewBlock` shouldn't ev
...
💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2580917867)
Thanks! Reworked.
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2580917867)
Thanks! Reworked.