💬 benthecarman commented on pull request "Fix dark mode detection on Linux":
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3324841313)
> Could you please clarify which build this change is intended to affect, statically linked or shared?
>
> If the latter, which system was this tested on (OS, desktop environment, Qt version)?
this sadly only effects dynamically linked builds.
I have tested on ubuntu 24.04 with Qt 6.4.2
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3324841313)
> Could you please clarify which build this change is intended to affect, statically linked or shared?
>
> If the latter, which system was this tested on (OS, desktop environment, Qt version)?
this sadly only effects dynamically linked builds.
I have tested on ubuntu 24.04 with Qt 6.4.2
⚠️ plebhash opened an issue: "`bitcoin-node` is unkillable after mining IPC connection is established"
(https://github.com/bitcoin/bitcoin/issues/33463)
I'm using `bitcoin-node` from [`v30.0rc1`](https://github.com/bitcoin/bitcoin/tree/v30.0rc1) to drive my development towards Sv2 integration over the new mining IPC interface https://github.com/stratum-mining/stratum/issues/1885
I noticed that after the first IPC connection is established, I'm unable to kill `bitcoin-node` via Ctrl+C.
<img width="1906" height="847" alt="Image" src="https://github.com/user-attachments/assets/97857485-2a8a-4f03-b388-73ed27c6d1d7" />
(https://github.com/bitcoin/bitcoin/issues/33463)
I'm using `bitcoin-node` from [`v30.0rc1`](https://github.com/bitcoin/bitcoin/tree/v30.0rc1) to drive my development towards Sv2 integration over the new mining IPC interface https://github.com/stratum-mining/stratum/issues/1885
I noticed that after the first IPC connection is established, I'm unable to kill `bitcoin-node` via Ctrl+C.
<img width="1906" height="847" alt="Image" src="https://github.com/user-attachments/assets/97857485-2a8a-4f03-b388-73ed27c6d1d7" />
💬 Sjors commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3324853126)
cc @ryanofsky
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3324853126)
cc @ryanofsky
💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3324859282)
It seems the new fuzz test in [16d5558](16d55585aa2d26e9177734927b8446faa72570e7)
caught a minor UndefinedBehaviorSanitizer: unsigned-integer-overflow issue on master.
```
/home/admin/actions-runner/_work/_temp/src/node/miner.cpp:414:47: runtime error: unsigned integer overflow: 2000 - 4000 cannot be represented in type 'size_t' (aka 'unsigned long')
#0 0x5b2b2753aee0 in node::BlockAssembler::addPackageTxs(int&, int&) /home/admin/actions-runner/_work/_temp/build/src/./node/miner.cpp
...
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3324859282)
It seems the new fuzz test in [16d5558](16d55585aa2d26e9177734927b8446faa72570e7)
caught a minor UndefinedBehaviorSanitizer: unsigned-integer-overflow issue on master.
```
/home/admin/actions-runner/_work/_temp/src/node/miner.cpp:414:47: runtime error: unsigned integer overflow: 2000 - 4000 cannot be represented in type 'size_t' (aka 'unsigned long')
#0 0x5b2b2753aee0 in node::BlockAssembler::addPackageTxs(int&, int&) /home/admin/actions-runner/_work/_temp/build/src/./node/miner.cpp
...
💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372950950)
You mean to only log enabled/disabled changes, but not enabled-after-assumevalid/enabled-no-burried changes? This was also deliberate to show the exact reason so that users understand the exact reason instead of just whether it's enabled or not.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372950950)
You mean to only log enabled/disabled changes, but not enabled-after-assumevalid/enabled-no-burried changes? This was also deliberate to show the exact reason so that users understand the exact reason instead of just whether it's enabled or not.
💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372950999)
The original message contained these indentations - expect in a very few cases. It seemed simpler to me to unify the text that way - otherwise I would have rewritten it completely since I don't particularly like the way it describes the situation.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372950999)
The original message contained these indentations - expect in a very few cases. It seemed simpler to me to unify the text that way - otherwise I would have rewritten it completely since I don't particularly like the way it describes the situation.
💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372951037)
I thought of this also, we can even add this error message as default and overwrite in every other case. If other reviewers also thing this would be simpler, I can do it in the next push, if needed.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372951037)
I thought of this also, we can even add this error message as default and overwrite in every other case. If other reviewers also thing this would be simpler, I can do it in the next push, if needed.
💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372951112)
that was the original suggestion as well, see https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351556972
I decided against it since I don't want to introduce an extra explicit assumevalid branch, it's still the same case (not part of av chain), just with better message - seems conceptually it's easier to grasp if we have fewer overall categories.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372951112)
that was the original suggestion as well, see https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351556972
I decided against it since I don't want to introduce an extra explicit assumevalid branch, it's still the same case (not part of av chain), just with better message - seems conceptually it's easier to grasp if we have fewer overall categories.
💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372951175)
I don't mind doing this in a separate PR, but here I have decided against it since it's not strictly related to the change, see: https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347916838
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2372951175)
I don't mind doing this in a separate PR, but here I have decided against it since it's not strictly related to the change, see: https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347916838
💬 plebhash commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3324880657)
the Rust code is still WIP, but FWIW, up until the ctrl+c, the interactions with `bitcoin-node` were:
- one call to `createNewBlock`
- one call to `waitTipChanged` (that hasn't been answered yet)
- all the bootstrapping to make the above possible (bootstrap RPC system, thread client, mining client)
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3324880657)
the Rust code is still WIP, but FWIW, up until the ctrl+c, the interactions with `bitcoin-node` were:
- one call to `createNewBlock`
- one call to `waitTipChanged` (that hasn't been answered yet)
- all the bootstrapping to make the above possible (bootstrap RPC system, thread client, mining client)
💬 PiRK commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#discussion_r2372969095)
Not sure why this commit was pulled into my PR the last time i rebased on master. This was the top commit from master that had just been merged (#33364) at the time
Some kind of github glitch?
(https://github.com/bitcoin/bitcoin/pull/33381#discussion_r2372969095)
Not sure why this commit was pulled into my PR the last time i rebased on master. This was the top commit from master that had just been merged (#33364) at the time
Some kind of github glitch?
💬 l0rinc commented on pull request "log: reduce excessive "rolling forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2372971637)
Agree, pushed a better solution to rate limit existing message + announcement of work ahead (as suggested by @ajtowns).
Rate limiting changes can be done in a separate PR now if needed.
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2372971637)
Agree, pushed a better solution to rate limit existing message + announcement of work ahead (as suggested by @ajtowns).
Rate limiting changes can be done in a separate PR now if needed.
💬 l0rinc commented on pull request "log: reduce excessive "rolling forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2372971804)
> Should be used with extreme care
Given that we didn't even have any rate limiting so far and as far as I can tell it wasn't weaponized so far I'm not sure this is an "extreme" danger.
But I don't mind removing the rate-limit related commit suggested by @ajtowns, since it seems to be more contentious than just reducing the rolling forward messages.
And I have extended it for the rolling forward messages as well and limit the log info to every 10k blocks to be less spammy.
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2372971804)
> Should be used with extreme care
Given that we didn't even have any rate limiting so far and as far as I can tell it wasn't weaponized so far I'm not sure this is an "extreme" danger.
But I don't mind removing the rate-limit related commit suggested by @ajtowns, since it seems to be more contentious than just reducing the rolling forward messages.
And I have extended it for the rolling forward messages as well and limit the log info to every 10k blocks to be less spammy.
💬 Sjors commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3324900365)
I'm not sure if `waitTipChanged` can handle being interrupted before its timeout yet, from either side. How does ctrl + c happens if you don't call `submitBlock`?
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3324900365)
I'm not sure if `waitTipChanged` can handle being interrupted before its timeout yet, from either side. How does ctrl + c happens if you don't call `submitBlock`?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3324906640)
Did the same benchmark with 5000 dbcache and there is a 6% speedup :rocket: - 4:27 vs 4:44. Even with far less cache misses this change still a benefit, and will continue to improve block connection speed as the blockchain and utxo set gets bigger.
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `echo 688c03597afb0b76077f1ffc4608eef19481056e && /usr/bin/time ./build/bin/bitcoind -printtoconsole=0 -connect=192.168.2.171 -stopatheight=912683 -dbcache=5000` |
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3324906640)
Did the same benchmark with 5000 dbcache and there is a 6% speedup :rocket: - 4:27 vs 4:44. Even with far less cache misses this change still a benefit, and will continue to improve block connection speed as the blockchain and utxo set gets bigger.
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `echo 688c03597afb0b76077f1ffc4608eef19481056e && /usr/bin/time ./build/bin/bitcoind -printtoconsole=0 -connect=192.168.2.171 -stopatheight=912683 -dbcache=5000` |
...
📝 mzumsande opened a pull request: "p2p: Use network-dependent timers for inbound inv scheduling"
(https://github.com/bitcoin/bitcoin/pull/33464)
Currently, `NextInvToInbounds` schedules each round of `inv` at the same time for all inbound peers. It's being done this way because with a separate timer per peer (like it's done for outbounds), an attacker could do multiple connections to learn about the time a transaction arrived. (#13298).
However, having a single timer for inbounds of all networks is also an obvious fingerprinting vector: Connecting to a suspected pair of privacy-netowrk and clearnet addresses and observing the `inv` p
...
(https://github.com/bitcoin/bitcoin/pull/33464)
Currently, `NextInvToInbounds` schedules each round of `inv` at the same time for all inbound peers. It's being done this way because with a separate timer per peer (like it's done for outbounds), an attacker could do multiple connections to learn about the time a transaction arrived. (#13298).
However, having a single timer for inbounds of all networks is also an obvious fingerprinting vector: Connecting to a suspected pair of privacy-netowrk and clearnet addresses and observing the `inv` p
...
💬 l0rinc commented on pull request "ci: run s390x job":
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3324911767)
> The issues it finds seems to happen less than once per year
For the record a big-endian CI machine would have helped with https://github.com/bitcoin/bitcoin/pull/31144
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3324911767)
> The issues it finds seems to happen less than once per year
For the record a big-endian CI machine would have helped with https://github.com/bitcoin/bitcoin/pull/31144
💬 ismaelsadeeq commented on issue "RFC: Bitcoin Core Node `BlockTemplateManager`":
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3324920650)
I opened #33421 last week and made an update to it today, I think it is ready for for review now.
The fuzz test added in the PR also caught an issue on master that should be backported to v30 see https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3324859282
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3324920650)
I opened #33421 last week and made an update to it today, I think it is ready for for review now.
The fuzz test added in the PR also caught an issue on master that should be backported to v30 see https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3324859282
💬 hebasto commented on pull request "Fix dark mode detection on Linux":
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3324959004)
> > Could you please clarify which build this change is intended to affect, statically linked or shared?
> > If the latter, which system was this tested on (OS, desktop environment, Qt version)?
>
> this sadly only effects dynamically linked builds.
Bringing this functionality to statically linked (including release) binaries would require introducing new build dependencies. There is a consensus among Core developers that this should only be considered after separating the GUI build into
...
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3324959004)
> > Could you please clarify which build this change is intended to affect, statically linked or shared?
> > If the latter, which system was this tested on (OS, desktop environment, Qt version)?
>
> this sadly only effects dynamically linked builds.
Bringing this functionality to statically linked (including release) binaries would require introducing new build dependencies. There is a consensus among Core developers that this should only be considered after separating the GUI build into
...
💬 maflcko commented on pull request "ci: remove Clang build from msan fuzz job":
(https://github.com/bitcoin/bitcoin/pull/33425#discussion_r2373034754)
yeah, but only to the extent of fixing the bug introduced in https://github.com/bitcoin/bitcoin/pull/33364/files by reverting the full and redundant clang compile it introduced.
(https://github.com/bitcoin/bitcoin/pull/33425#discussion_r2373034754)
yeah, but only to the extent of fixing the bug introduced in https://github.com/bitcoin/bitcoin/pull/33364/files by reverting the full and redundant clang compile it introduced.