✅ fanquake closed a pull request: "RPC/Wallet: Add "use_txids" to output of getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/22693)
(https://github.com/bitcoin/bitcoin/pull/22693)
👍 MarcoFalke approved a pull request: "ci, iwyu: Update mappings"
(https://github.com/bitcoin/bitcoin/pull/27710#pullrequestreview-1449308791)
lgtm ACK
(https://github.com/bitcoin/bitcoin/pull/27710#pullrequestreview-1449308791)
lgtm ACK
💬 MarcoFalke commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209195309)
Would it make sense to upstream those?
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209195309)
Would it make sense to upstream those?
💬 fanquake commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209196694)
Was wondering the same about `boost-1.75-all.imp` and `qt5_11.imp`, given they are clearly for older versions. Are we going to upstream updates, or just wait until the mappings break?
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209196694)
Was wondering the same about `boost-1.75-all.imp` and `qt5_11.imp`, given they are clearly for older versions. Are we going to upstream updates, or just wait until the mappings break?
💬 hebasto commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209200447)
Yes, I did consider upstreaming these change. It seems make sense to do it after removing all Boost and Qt related warnings for our code, to be sure that the changes are complete.
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209200447)
Yes, I did consider upstreaming these change. It seems make sense to do it after removing all Boost and Qt related warnings for our code, to be sure that the changes are complete.
💬 MarcoFalke commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209217111)
If you want to look for missing ones, there should be at least signals2? See https://api.cirrus-ci.com/v1/task/4955079116062720/logs/ci.log with search string "+#include <boos"
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1209217111)
If you want to look for missing ones, there should be at least signals2? See https://api.cirrus-ci.com/v1/task/4955079116062720/logs/ci.log with search string "+#include <boos"
📝 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
...