💬 theStack commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#discussion_r1795181976)
Fixed now.
(https://github.com/bitcoin/bitcoin/pull/30937#discussion_r1795181976)
Fixed now.
💬 naumenkogs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2404771253)
ACK ab45a0654c189da3013ef0c8d30478601052d922
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2404771253)
ACK ab45a0654c189da3013ef0c8d30478601052d922
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1795202925)
> Addressed in https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346.
How was it addressed? I still see the same output using 2d009aa8e861c161820a4c0619ecd4b2329267af:
```bash
make -C depends -j12 HOST=x86_64-apple-darwin
gmake: Entering directory '/bitcoin/depends'
Fetching qtbase-everywhere-src-6.7.3.tar.xz from https://download.qt.io/official_releases/qt/6.7/6.7.3/submodules
% Total % Received % Xferd Average Speed Time Time Time Current
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1795202925)
> Addressed in https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346.
How was it addressed? I still see the same output using 2d009aa8e861c161820a4c0619ecd4b2329267af:
```bash
make -C depends -j12 HOST=x86_64-apple-darwin
gmake: Entering directory '/bitcoin/depends'
Fetching qtbase-everywhere-src-6.7.3.tar.xz from https://download.qt.io/official_releases/qt/6.7/6.7.3/submodules
% Total % Received % Xferd Average Speed Time Time Time Current
...
💬 maflcko commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2404783263)
> I think we could consider removing the honggfuzz netdriver instructions in a follow
Not sure about touching real code with the motivation to make instructions happy that may be removed anyway.
If the changes are meaningful for the fuzz target, then it may be fine.
> It also makes the `p2p_transport_serialization` fuzz target simpler since we do not need to assist the network magic and checksum creation anymore.
Can you provide more details here? IIUC a hash will still be calculated
...
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2404783263)
> I think we could consider removing the honggfuzz netdriver instructions in a follow
Not sure about touching real code with the motivation to make instructions happy that may be removed anyway.
If the changes are meaningful for the fuzz target, then it may be fine.
> It also makes the `p2p_transport_serialization` fuzz target simpler since we do not need to assist the network magic and checksum creation anymore.
Can you provide more details here? IIUC a hash will still be calculated
...
👍 itornaza approved a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2360026747)
Concept ACK ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9
Enriches the functional tests for the the `getorphantxs` rpc and offers added functionality to conveniently access the expiration times of transactions in the orphanage.
Broadening the context with refactoring from `rpc_getorphantxs.py` to `rpc_orphans.py` as a future provision for more orphanage rpcs looks good to me.
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2360026747)
Concept ACK ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9
Enriches the functional tests for the the `getorphantxs` rpc and offers added functionality to conveniently access the expiration times of transactions in the orphanage.
Broadening the context with refactoring from `rpc_getorphantxs.py` to `rpc_orphans.py` as a future provision for more orphanage rpcs looks good to me.
💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1795269088)
To extend a bit: Previously (and after my pull right now), it was easy to toggle between translation and non-translation by replacing `_` with `Untranslated` (or vice-versa):
```diff
-strprintf(Untranslated("Cannot create database file %s"), filename);
+strprintf( _("Cannot create database file %s"), filename);
```
With your change, the function call order has to be changed as well.
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1795269088)
To extend a bit: Previously (and after my pull right now), it was easy to toggle between translation and non-translation by replacing `_` with `Untranslated` (or vice-versa):
```diff
-strprintf(Untranslated("Cannot create database file %s"), filename);
+strprintf( _("Cannot create database file %s"), filename);
```
With your change, the function call order has to be changed as well.
💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2404888004)
I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don't mind.
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2404888004)
I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don't mind.
💬 fanquake commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2404945576)
Guix Build:
```bash
9cf65a99fa046a1a3acbee14d6dec658b9fe7f4e55a4b6f50d063ebb4029e3eb guix-build-882f736d0a60/output/aarch64-linux-gnu/SHA256SUMS.part
46780332298b988bc9b4c2903d077f49978dc92250cf5221557ce4459383b18e guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu-debug.tar.gz
72362b6f0394f4b8b1e69e4520c892d5b3e9f3d86788c8f0ee59f2b0d9e51717 guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu.tar.gz
c4b5294efed908af
...
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2404945576)
Guix Build:
```bash
9cf65a99fa046a1a3acbee14d6dec658b9fe7f4e55a4b6f50d063ebb4029e3eb guix-build-882f736d0a60/output/aarch64-linux-gnu/SHA256SUMS.part
46780332298b988bc9b4c2903d077f49978dc92250cf5221557ce4459383b18e guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu-debug.tar.gz
72362b6f0394f4b8b1e69e4520c892d5b3e9f3d86788c8f0ee59f2b0d9e51717 guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu.tar.gz
c4b5294efed908af
...
👍 fanquake approved a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2360160618)
ACK 882f736d0a607976ee5e3a6cbcb5385524bc72c6
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2360160618)
ACK 882f736d0a607976ee5e3a6cbcb5385524bc72c6
🚀 fanquake merged a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937)
(https://github.com/bitcoin/bitcoin/pull/30937)
💬 dergoegge commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2405012504)
> I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don't mind.
The LLVM approach also supports that:

(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2405012504)
> I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don't mind.
The LLVM approach also supports that:

💬 ismaelsadeeq commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405065185)
This issue was opened to track the effort to improve the Bitcoin core fee estimator. This comment will provide information on the status of this project and some key ideas on how to achieve this.
**Completed Tasks so far:**
- **Enabled asynchronous block policy fee estimator updates**: By making it a client of the validation interface.
- **Empirical data analysis**: To determine which forecasting strategy provides the most accurate estimates while withstanding common attack vectors.
- De
...
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405065185)
This issue was opened to track the effort to improve the Bitcoin core fee estimator. This comment will provide information on the status of this project and some key ideas on how to achieve this.
**Completed Tasks so far:**
- **Enabled asynchronous block policy fee estimator updates**: By making it a client of the validation interface.
- **Empirical data analysis**: To determine which forecasting strategy provides the most accurate estimates while withstanding common attack vectors.
- De
...
💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405071446)
> Can you provide more details here? IIUC a hash will still be calculated, just not compared, so this may only drop one hash call, with a possible maximum speed-up of 2x. However, it would be good to actually measure the difference. Otherwise, I am not sure if this is worth it.
This is not about performance. It makes the `p2p_transport_serialization` fuzz target simpler because we can remove the code that assists the checksum and magic bytes creation. So, we can achieve the same with less LoC
...
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405071446)
> Can you provide more details here? IIUC a hash will still be calculated, just not compared, so this may only drop one hash call, with a possible maximum speed-up of 2x. However, it would be good to actually measure the difference. Otherwise, I am not sure if this is worth it.
This is not about performance. It makes the `p2p_transport_serialization` fuzz target simpler because we can remove the code that assists the checksum and magic bytes creation. So, we can achieve the same with less LoC
...
💬 maflcko commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405082968)
Ok, then I am nack-ish on this. Generally, test code should follow real code, not the other way round, unless there is a reason. Saving 15 lines of test-only code seems a weak reason to me, but I don't mind if others like this.
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405082968)
Ok, then I am nack-ish on this. Generally, test code should follow real code, not the other way round, unless there is a reason. Saving 15 lines of test-only code seems a weak reason to me, but I don't mind if others like this.
💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405088673)
Since everyone agrees on removing Honggfuzz netdriver, I think we can change the approach to simply just remove it from the documentation.
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405088673)
Since everyone agrees on removing Honggfuzz netdriver, I think we can change the approach to simply just remove it from the documentation.
💬 jonatack commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405100792)
> Create estimatefee RPC
Four years ago I was beginning to implement the roadmap as described in https://github.com/bitcoin/bitcoin/pull/20484#issuecomment-734786305, beginning with a `setfeerate` RPC in https://github.com/bitcoin/bitcoin/pull/20391 and a planned `estimatefeerate` RPC, both using sat/vB units. Was considering picking these up again. Presumably orthogonal, apart from coordinating on the RPC naming.
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405100792)
> Create estimatefee RPC
Four years ago I was beginning to implement the roadmap as described in https://github.com/bitcoin/bitcoin/pull/20484#issuecomment-734786305, beginning with a `setfeerate` RPC in https://github.com/bitcoin/bitcoin/pull/20391 and a planned `estimatefeerate` RPC, both using sat/vB units. Was considering picking these up again. Presumably orthogonal, apart from coordinating on the RPC naming.
🤔 TheCharlatan reviewed a pull request: "log: Enforce trailing newline"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2360363711)
Post-merge ACK
I think this really showed the limits of having our own clang-tidy checks. They don't lend themselves well to code that is evolving, may receive renames, or new methods.
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2360363711)
Post-merge ACK
I think this really showed the limits of having our own clang-tidy checks. They don't lend themselves well to code that is evolving, may receive renames, or new methods.
💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1795456841)
Per feedback in https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759115295, begin this sentence with "Assuming the build directory is named `build`, running..."
I agreed with the feedback not to add it where it was already mentioned in other files, but in this file there is no other mention.
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1795456841)
Per feedback in https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759115295, begin this sentence with "Assuming the build directory is named `build`, running..."
I agreed with the feedback not to add it where it was already mentioned in other files, but in this file there is no other mention.
🤔 jonatack reviewed a pull request: "doc: cmake: prepend "build" to functional/test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2360372434)
LGTM modulo comment
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2360372434)
LGTM modulo comment
👍 TheCharlatan approved a pull request: "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment"
(https://github.com/bitcoin/bitcoin/pull/30666#pullrequestreview-2360426595)
Re-ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992
(https://github.com/bitcoin/bitcoin/pull/30666#pullrequestreview-2360426595)
Re-ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992