💬 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
💬 maflcko commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1795480094)
It seems `CLIENT_NAME` is already used in a different (non-build) context to denote the "user agent name". Maybe a preparatory commit could be added to change that to `UA_NAME`, for clarity and to avoid a name clash?
```
$ git grep --line-number CLIENT_NAME
src/clientversion.cpp:23:const std::string CLIENT_NAME("Satoshi");
src/clientversion.h:36:extern const std::string CLIENT_NAME;
src/init.cpp:1470: strSubVersion = FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, uacomments);
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1795480094)
It seems `CLIENT_NAME` is already used in a different (non-build) context to denote the "user agent name". Maybe a preparatory commit could be added to change that to `UA_NAME`, for clarity and to avoid a name clash?
```
$ git grep --line-number CLIENT_NAME
src/clientversion.cpp:23:const std::string CLIENT_NAME("Satoshi");
src/clientversion.h:36:extern const std::string CLIENT_NAME;
src/init.cpp:1470: strSubVersion = FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, uacomments);
💬 TheCharlatan commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2405170985)
Post-merge ACK 882f736d0a60
Guix build (aarch64):
```
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-aarch
...
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2405170985)
Post-merge ACK 882f736d0a60
Guix build (aarch64):
```
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-aarch
...
💬 ismaelsadeeq commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405172852)
> Both using sat/vB units.
This is a good idea.
> Beginning with a setfeerate RPC in https://github.com/bitcoin/bitcoin/pull/20391.
I learned about this issue after @murchandamus's review comment https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1526664705. I think we should just unify `settxfee` and your `setfeerate` into one RPC?
> Was considering picking these up again. Presumably orthogonal, apart from coordinating on the RPC naming.
Happy to review when you do.
...
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405172852)
> Both using sat/vB units.
This is a good idea.
> Beginning with a setfeerate RPC in https://github.com/bitcoin/bitcoin/pull/20391.
I learned about this issue after @murchandamus's review comment https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1526664705. I think we should just unify `settxfee` and your `setfeerate` into one RPC?
> Was considering picking these up again. Presumably orthogonal, apart from coordinating on the RPC naming.
Happy to review when you do.
...
💬 fanquake commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1795498723)
I think if we end up doingthat, we should extend this to renaming other source variables that exist in the PACKAGE_ namespace. This PR as-is is already changing things such that naming becomes more inconsistent in that regard.
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1795498723)
I think if we end up doingthat, we should extend this to renaming other source variables that exist in the PACKAGE_ namespace. This PR as-is is already changing things such that naming becomes more inconsistent in that regard.
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1795560402)
this is definitely problematic, but I can't seem to figure out a way to hit it. In prod this would "just" cause it to not call `m_txrequest.ForgetTxHash(ptx->GetWitnessHash());` for it, but would have to be in a 1p1c package setting.
To retain current behavior, I think it can just be expanded to
```suggestion
// DIFFERENT_WITNESS has "valid" status, but we still want to stop requesting it
if (!Assume(state.IsInvalid() ||
tx_result.m_result_type == MempoolAcceptResult::Res
...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1795560402)
this is definitely problematic, but I can't seem to figure out a way to hit it. In prod this would "just" cause it to not call `m_txrequest.ForgetTxHash(ptx->GetWitnessHash());` for it, but would have to be in a 1p1c package setting.
To retain current behavior, I think it can just be expanded to
```suggestion
// DIFFERENT_WITNESS has "valid" status, but we still want to stop requesting it
if (!Assume(state.IsInvalid() ||
tx_result.m_result_type == MempoolAcceptResult::Res
...
📝 danielabrozzoni opened a pull request: "rest: Support transaction broadcast in REST interface"
(https://github.com/bitcoin/bitcoin/pull/31065)
This adds a REST endpoint to allow broadcasting a transaction:
`POST /rest/broadcast.hex`
The transaction hex must be passed in the body of the request;
on success, the txid of the broadcasted transaction will be returned.
Fixes #31017
(https://github.com/bitcoin/bitcoin/pull/31065)
This adds a REST endpoint to allow broadcasting a transaction:
`POST /rest/broadcast.hex`
The transaction hex must be passed in the body of the request;
on success, the txid of the broadcasted transaction will be returned.
Fixes #31017
💬 ryanofsky commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2405280136)
> Is it ok for other external users of `processNewBlock` to skip over the pre-checks and optimizations that are normally done in `submitblock`?
I assumed it was ok, but now that `interfaces::BlockTemplate::submitSolution` in added in 525e9dcba0b8c6744bcd3725864f39786afc8ed5 (to deal with the concern in https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2229233817) maybe the `interfaces::Mining::processNewBlock` method could be dropped and RPC code can go back to calling `ProcessNewBlo
...
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2405280136)
> Is it ok for other external users of `processNewBlock` to skip over the pre-checks and optimizations that are normally done in `submitblock`?
I assumed it was ok, but now that `interfaces::BlockTemplate::submitSolution` in added in 525e9dcba0b8c6744bcd3725864f39786afc8ed5 (to deal with the concern in https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2229233817) maybe the `interfaces::Mining::processNewBlock` method could be dropped and RPC code can go back to calling `ProcessNewBlo
...
💬 glozow commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1795517895)
tiny typo unusued
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1795517895)
tiny typo unusued
🤔 glozow reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2360504869)
ACK 0b3ec8c59b2b6d598531cd4e688eb276e597c825, reviewed range-diff from aab53ddcd8fcbc3c0be0da9383f8e06abe5badda and `clusterlin_depgraph_sim`
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2360504869)
ACK 0b3ec8c59b2b6d598531cd4e688eb276e597c825, reviewed range-diff from aab53ddcd8fcbc3c0be0da9383f8e06abe5badda and `clusterlin_depgraph_sim`
💬 glozow commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1795548751)
Nice, no more naming clash for `Cluster` in my mind
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1795548751)
Nice, no more naming clash for `Cluster` in my mind
🚀 glozow merged a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857)
(https://github.com/bitcoin/bitcoin/pull/30857)
⚠️ maflcko opened an issue: " scriptpubkeyman fuzz target TopUp is slow (3/N)"
(https://github.com/bitcoin/bitcoin/issues/31066)
Follow-up to https://github.com/bitcoin/bitcoin/issues/30476 and https://github.com/bitcoin/bitcoin/issues/30541:
Found by OSS-Fuzz: https://issues.oss-fuzz.com/issues/369374541
Probably a duplicate of, or similarity with https://github.com/bitcoin/bitcoin/issues/30498
File: [clusterfuzz-testcase-scriptpubkeyman-6582005385461760.not.txt](https://github.com/user-attachments/files/17330258/clusterfuzz-testcase-scriptpubkeyman-6582005385461760.not.txt)
```
$ FUZZ=scriptpubkeyman pe
...
(https://github.com/bitcoin/bitcoin/issues/31066)
Follow-up to https://github.com/bitcoin/bitcoin/issues/30476 and https://github.com/bitcoin/bitcoin/issues/30541:
Found by OSS-Fuzz: https://issues.oss-fuzz.com/issues/369374541
Probably a duplicate of, or similarity with https://github.com/bitcoin/bitcoin/issues/30498
File: [clusterfuzz-testcase-scriptpubkeyman-6582005385461760.not.txt](https://github.com/user-attachments/files/17330258/clusterfuzz-testcase-scriptpubkeyman-6582005385461760.not.txt)
```
$ FUZZ=scriptpubkeyman pe
...