📝 MarcoFalke opened a pull request: "refactor: Add [[nodiscard]] where ignoring a Result return type is an error "
(https://github.com/bitcoin/bitcoin/pull/27774)
Only add it for those where it is an error to ignore. Also, fix the gcc compile warning https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1564350880. Also, fix iwyu for touched header files.
(https://github.com/bitcoin/bitcoin/pull/27774)
Only add it for those where it is an error to ignore. Also, fix the gcc compile warning https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1564350880. Also, fix iwyu for touched header files.
💬 hebasto commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209239987)
I've skipped non-test Boost stuff intentionally to avoid potential conflicts with other (more important) PRs.
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209239987)
I've skipped non-test Boost stuff intentionally to avoid potential conflicts with other (more important) PRs.
💬 MarcoFalke commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209243376)
`contrib/devtools/iwyu/bitcoin.core.imp` isn't modified by anyone else, so shouldn't cause any (silent) merge conflicts?
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209243376)
`contrib/devtools/iwyu/bitcoin.core.imp` isn't modified by anyone else, so shouldn't cause any (silent) merge conflicts?
💬 joostjager commented on pull request "Make non-standard tx acceptance a peer option.":
(https://github.com/bitcoin/bitcoin/pull/27772#issuecomment-1567067889)
>Rather than open more PRs that are duplicative of other recently rejected PRs, it would be more helpful to directly address the feedback that was previously given.
This PR was intended as a draft. The goal was not to propose any changes right away, but rather to illustrate (also for myself) the touch points in the codebase. I wouldn't consider this PR a duplicate though. It only relaxes tx acceptance on the rpc level, and leaves the p2p restrictions in place - contrary to #27578. Nevertheles
...
(https://github.com/bitcoin/bitcoin/pull/27772#issuecomment-1567067889)
>Rather than open more PRs that are duplicative of other recently rejected PRs, it would be more helpful to directly address the feedback that was previously given.
This PR was intended as a draft. The goal was not to propose any changes right away, but rather to illustrate (also for myself) the touch points in the codebase. I wouldn't consider this PR a duplicate though. It only relaxes tx acceptance on the rpc level, and leaves the p2p restrictions in place - contrary to #27578. Nevertheles
...
🚀 fanquake merged a pull request: "contrib/init: Better systemd integration"
(https://github.com/bitcoin/bitcoin/pull/25975)
(https://github.com/bitcoin/bitcoin/pull/25975)
💬 hebasto commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#issuecomment-1567109899)
Addressed @MarcoFalke's https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209217111.
(https://github.com/bitcoin/bitcoin/pull/27710#issuecomment-1567109899)
Addressed @MarcoFalke's https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209217111.
💬 hebasto commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209296024)
> If you want to look for missing ones, there should be at least signals2? See [api.cirrus-ci.com/v1/task/4955079116062720/logs/ci.log](https://api.cirrus-ci.com/v1/task/4955079116062720/logs/ci.log) with search string "+#include <boos"
Thanks! [Done](https://github.com/bitcoin/bitcoin/pull/27710#issuecomment-1567109899).
> `contrib/devtools/iwyu/bitcoin.core.imp` isn't modified by anyone else, so shouldn't cause any (silent) merge conflicts?
You're right. I meant actual adding/removing
...
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209296024)
> If you want to look for missing ones, there should be at least signals2? See [api.cirrus-ci.com/v1/task/4955079116062720/logs/ci.log](https://api.cirrus-ci.com/v1/task/4955079116062720/logs/ci.log) with search string "+#include <boos"
Thanks! [Done](https://github.com/bitcoin/bitcoin/pull/27710#issuecomment-1567109899).
> `contrib/devtools/iwyu/bitcoin.core.imp` isn't modified by anyone else, so shouldn't cause any (silent) merge conflicts?
You're right. I meant actual adding/removing
...
💬 TheCharlatan commented on pull request "refactor: Add [[nodiscard]] where ignoring a Result return type is an error":
(https://github.com/bitcoin/bitcoin/pull/27774#issuecomment-1567112224)
> Only add it for those where it is an error to ignore.
How did you select these?
(https://github.com/bitcoin/bitcoin/pull/27774#issuecomment-1567112224)
> Only add it for those where it is an error to ignore.
How did you select these?
💬 joostjager commented on issue "Allow accepting non-standard transactions on mainnet via local rpc":
(https://github.com/bitcoin/bitcoin/issues/27768#issuecomment-1567115600)
@sdaftuar wrote in https://github.com/bitcoin/bitcoin/pull/27772#issuecomment-1566232510:
> Are you unaware that some of the DoS attacks that are prevented by our standardness checks include situations where a block might be mined that would take a very long time to validate, a cost that would be felt by the whole network? See for instance [https://bitcointalk.org/?topic=140078%7CCVE-2013-2292]]](https://bitcointalk.org/?topic=140078%7CCVE-2013-2292%5D%5D). It is absurd that this issue would
...
(https://github.com/bitcoin/bitcoin/issues/27768#issuecomment-1567115600)
@sdaftuar wrote in https://github.com/bitcoin/bitcoin/pull/27772#issuecomment-1566232510:
> Are you unaware that some of the DoS attacks that are prevented by our standardness checks include situations where a block might be mined that would take a very long time to validate, a cost that would be felt by the whole network? See for instance [https://bitcointalk.org/?topic=140078%7CCVE-2013-2292]]](https://bitcointalk.org/?topic=140078%7CCVE-2013-2292%5D%5D). It is absurd that this issue would
...
💬 MarcoFalke commented on pull request "refactor: Add [[nodiscard]] where ignoring a Result return type is an error":
(https://github.com/bitcoin/bitcoin/pull/27774#issuecomment-1567120817)
> How did you select these?
I asked my brain if it thought that ignoring the return type would lead to errors in later code.
(https://github.com/bitcoin/bitcoin/pull/27774#issuecomment-1567120817)
> How did you select these?
I asked my brain if it thought that ignoring the return type would lead to errors in later code.
💬 MarcoFalke commented on pull request "Make non-standard tx acceptance a peer option.":
(https://github.com/bitcoin/bitcoin/pull/27772#discussion_r1209308134)
Looks like you just switch this from true to false here, without making it an option. NACK on making this a silent footgun for all users.
(https://github.com/bitcoin/bitcoin/pull/27772#discussion_r1209308134)
Looks like you just switch this from true to false here, without making it an option. NACK on making this a silent footgun for all users.
💬 joostjager commented on pull request "Make non-standard tx acceptance a peer option.":
(https://github.com/bitcoin/bitcoin/pull/27772#discussion_r1209312388)
Are you suggesting to add `-acceptnonstdtxnrpc`?
(https://github.com/bitcoin/bitcoin/pull/27772#discussion_r1209312388)
Are you suggesting to add `-acceptnonstdtxnrpc`?
💬 fanquake commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1567147860)
> I end up with three files / folders:
This is the same as the current build process, where you end up with:
1. Bitcoin-Qt.app (which Finder displays as "Bitcoin Core")
2. Bitcoin-Core.dmg
3. dist/Bitcoin-Qt.app
Happy to refine/rework this further in a follow up, but don't really want to start changing this now, as it's not new behaviour.
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1567147860)
> I end up with three files / folders:
This is the same as the current build process, where you end up with:
1. Bitcoin-Qt.app (which Finder displays as "Bitcoin Core")
2. Bitcoin-Core.dmg
3. dist/Bitcoin-Qt.app
Happy to refine/rework this further in a follow up, but don't really want to start changing this now, as it's not new behaviour.
💬 fanquake commented on pull request "Make non-standard tx acceptance a peer option.":
(https://github.com/bitcoin/bitcoin/pull/27772#issuecomment-1567155749)
> I will reply to your other feedback points within the context of https://github.com/bitcoin/bitcoin/issues/27768, to avoid spreading out the discussion across multiple places.
I think it'd be better to close this PR for now, (I agree with suhas's comments above), and continue discussion in #27768 (I see [you've responded there](https://github.com/bitcoin/bitcoin/issues/27768#issuecomment-1567115600) with further details of what you are interested in).
I don't think there's a need to keep
...
(https://github.com/bitcoin/bitcoin/pull/27772#issuecomment-1567155749)
> I will reply to your other feedback points within the context of https://github.com/bitcoin/bitcoin/issues/27768, to avoid spreading out the discussion across multiple places.
I think it'd be better to close this PR for now, (I agree with suhas's comments above), and continue discussion in #27768 (I see [you've responded there](https://github.com/bitcoin/bitcoin/issues/27768#issuecomment-1567115600) with further details of what you are interested in).
I don't think there's a need to keep
...
👍 TheCharlatan approved a pull request: "refactor: Add [[nodiscard]] where ignoring a Result return type is an error"
(https://github.com/bitcoin/bitcoin/pull/27774#pullrequestreview-1449518089)
ACK fa5680b7520c340952239c4e25ebe505d16c7c39
(https://github.com/bitcoin/bitcoin/pull/27774#pullrequestreview-1449518089)
ACK fa5680b7520c340952239c4e25ebe505d16c7c39
💬 furszy commented on pull request "Fix rpc_getblockfrompeer intermittency by introducing 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1567161622)
> > I started there but.. do we really care?
>
> We definitely care enough to investigate further, before adding a new RPC command to fix an intermittent issue in a functional test.
>
> Looks like @theStack has done that investigation here: [#27749 (comment)](https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566354933), and @mzumsande has suggested a potential alternative fix: [#27749 (comment)](https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566554075):
>
> >
...
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1567161622)
> > I started there but.. do we really care?
>
> We definitely care enough to investigate further, before adding a new RPC command to fix an intermittent issue in a functional test.
>
> Looks like @theStack has done that investigation here: [#27749 (comment)](https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566354933), and @mzumsande has suggested a potential alternative fix: [#27749 (comment)](https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566554075):
>
> >
...
✅ joostjager closed a pull request: "Make non-standard tx acceptance a peer option."
(https://github.com/bitcoin/bitcoin/pull/27772)
(https://github.com/bitcoin/bitcoin/pull/27772)
💬 ajtowns commented on issue "Allow accepting non-standard transactions on mainnet via local rpc":
(https://github.com/bitcoin/bitcoin/issues/27768#issuecomment-1567174457)
> The reason that I am interested in non-standard transactions is the taproot annex. There seem to be many ways to benefit from this field, and it might also reduce chain space requirements for certain applications.
The way to fix that isn't to standardise uses of the annex, not to change the node to accept anything into the mempool / relay anything. See also https://github.com/bitcoin/bips/pull/1381 and https://github.com/bitcoin-inquisition/bitcoin/pull/22
(https://github.com/bitcoin/bitcoin/issues/27768#issuecomment-1567174457)
> The reason that I am interested in non-standard transactions is the taproot annex. There seem to be many ways to benefit from this field, and it might also reduce chain space requirements for certain applications.
The way to fix that isn't to standardise uses of the annex, not to change the node to accept anything into the mempool / relay anything. See also https://github.com/bitcoin/bips/pull/1381 and https://github.com/bitcoin-inquisition/bitcoin/pull/22
💬 MarcoFalke commented on pull request "Fix `#include`s in `src/wallet`":
(https://github.com/bitcoin/bitcoin/pull/27759#issuecomment-1567205314)
lgtm ACK 1f97572b9c0d339a8497340e7066050aba9d7694
(https://github.com/bitcoin/bitcoin/pull/27759#issuecomment-1567205314)
lgtm ACK 1f97572b9c0d339a8497340e7066050aba9d7694
💬 furszy commented on issue "rpc_getblockfrompeer.py intermittent failure: assert_equal(pruneheight, 248); not(249 == 248)":
(https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1567211356)
Awesome investigation! It feels good when all dots are connected.
Apart from mzumzande's patch to enforce sequential block storage, which looks promising at first glance. I would like to refer to a portion of what I just wrote in https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1567161622:
I believe the main conclusion of the analysis (at least mine) is that the accuracy of the `pruneblockchain` result is questionable. The real issue lies in the fact that the RPC command's result
...
(https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1567211356)
Awesome investigation! It feels good when all dots are connected.
Apart from mzumzande's patch to enforce sequential block storage, which looks promising at first glance. I would like to refer to a portion of what I just wrote in https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1567161622:
I believe the main conclusion of the analysis (at least mine) is that the accuracy of the `pruneblockchain` result is questionable. The real issue lies in the fact that the RPC command's result
...