💬 MarcoFalke commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1221581710)
I think `const` in this context is mainly used to force the designated initializer to initialize the field, because no constructor exists and otherwise this may end up being uninitialized?

💬 dergoegge commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#discussion_r1221594636)
```suggestion
void ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const BlockTransactions& block_transactions)
```

I would prefer this order of the params (matches the other methods) and the `BlockTransactions` should be marked const.

💬 MarcoFalke commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1221598752)
Yes, the string is eaten by the help generator either way

💬 MarcoFalke commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1580838529)
Needs rebase?

💬 ajtowns commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1221620278)
Seems like it would be better to explicitly specify defaults (`false` and `0`?) in that case? If you want to delete the default constructor to force aggregate initialization, then `delta_info() = delete;` seems more direct?

💬 MarcoFalke commented on pull request "fuzz: Partially revert #27780":
(https://github.com/bitcoin/bitcoin/pull/27810#issuecomment-1580855183)
nice lgtm ACK 71200ac390fe5c10f088cbe8053b010b515757b1

📝 theuni reopened a pull request: "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types"
(https://github.com/bitcoin/bitcoin/pull/26296)
Demo of integrating the bitcoin-tidy (https://github.com/theuni/bitcoin-tidy/), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job. The plugin currently has a single check (`bitcoin-init-list`), the purpose of which is to warn when non-defaulted fields in an aggregate type are left uninitialized.

The REVERT ME commit produces the following output with the check enabled:
```bash
clang-tidy-14 --use-color -p=/home/ubuntu/ci_scratch/ci/scratch/
...

💬 theuni commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1580877766)
Reopened as I'd like to get the infrastructure for this hooked up.

I think the issue here was disagreement about what the initial plugin should do. @MarcoFalke Do you have any suggestions for an uncontroversial "hello world" type plugin just to get started with something?

🚀 fanquake merged a pull request: "fuzz: Partially revert #27780"
(https://github.com/bitcoin/bitcoin/pull/27810)

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1580895192)
done, cherry-picked 🚜.

📝 MarcoFalke opened a pull request: "ci: Nuke Android APK task, Use credits for tsan"
(https://github.com/bitcoin/bitcoin/pull/27834)
The Android task has many issues:

* It runs into more network timeouts (intermittent failures) than other tasks
* It never failed in its existence when all other tasks passes, thus it is useless (so far)

Fix all issues by removing the task. Note that the CI env file is kept, so anyone can still run the Android CI.

Also, use the compute credits to promote tsan, a more useful task.

💬 fanquake commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1580900804)
Concept ACK

💬 instagibbs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1580978617)
re ACK https://github.com/bitcoin/bitcoin/pull/26969/commits/686629d2be5545ef59cf0e97f4f3a74c6cde2efa

only differences beyond rebase is suggested changes for `ProcessCompactBlockTxns` argument ordering and const-ness of BlockTransactions arg.

💬 brunoerg commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#discussion_r1221729333)
Make sense. I'm going to address it

💬 brunoerg commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1580980099)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/26969#discussion_r1221594636

💬 MarcoFalke commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1581058082)
> In other words, imo this PR fixes a bug.

Not sure. I don't think it makes sense to change the behavior of the `-datacarrier` setting. There is no reason for it to exist in the first place and it should just be removed. It is redundant with the `-datacarriersize` setting, because setting `-datacarriersize=0` is equivalent. Having two ways to achieve the same is just confusing and can lead to issues.
Also, unrelated to removing the option, the `std::optional<unsigned>` should just be flatten
...

👍 dergoegge approved a pull request: "ci: Nuke Android APK task, Use credits for tsan"
(https://github.com/bitcoin/bitcoin/pull/27834#pullrequestreview-1467998616)
ACK fa22538e481fa2c4f0b5d6f91166335e60b67fe9 - nuke it

I have only ever seen this task pass (even if all others fail) or fail intermittently.

📝 Sjors opened a pull request: "doc: clarify full Xcode download is not needed"
(https://github.com/bitcoin/bitcoin/pull/27835)

💬 grdddj commented on pull request "doc: clarify full Xcode download is not needed":
(https://github.com/bitcoin/bitcoin/pull/27835#issuecomment-1581067617)
LGTM!

💬 MarcoFalke commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581091453)
Not sure what is up with CI, but lint says:

```
contrib/devtools/special-instruction-check.py:55: error: "Binary" has no attribute "has_section" [attr-defined]
```

Maybe rebase?