💬 petertodd commented on pull request "doc: Clarify -datacarriersize, add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221516504)
Since you are updating this, you should change the doc text to make clear that the current behavior is a mistake that needs fixing rathre than continue to incorrectly call them data carrier scriptPubKey's.

But it'd be less code churn to just fix the behavior in one shot. We have an ACK on https://github.com/bitcoin/bitcoin/pull/27261, so there is an argument to fix the bare OP_Return case. Rather than merging https://github.com/bitcoin/bitcoin/pull/27261 as-is I could make a pull-req that sim
...

💬 MarcoFalke commented on pull request "doc: Clarify -datacarriersize, add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#discussion_r1221522467)
> Fixing that error would also allow -datacarrier to be dropped entirely, as it is redundant: -datacarriersize=0 prohibits data carrying outputs just fine.

I think `-datacarrier` can be dropped regardless of whether the behavior is changed. And this would have been my review comment in https://github.com/bitcoin/bitcoin/pull/27261 :)

💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1580738352)
Needs rebase?

💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1580740902)
Thanks, I pushed your branch (rebased)

💬 fanquake commented on issue "Alternative read only paths for -blocksdir":
(https://github.com/bitcoin/bitcoin/issues/27833#issuecomment-1580742109)
Agree with Marco. You can use something like LVM to combine your drives, and then point to a single blocksdir. There's no need for us to introduce complexity to mange this into Bitcoin Core.

✅ fanquake closed an issue: "Alternative read only paths for -blocksdir"
(https://github.com/bitcoin/bitcoin/issues/27833)

💬 ryanofsky commented on pull request "kernel: Remove shutdown globals from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869)
Thanks for following up and incorporating the feedback!

On the topic of how to move forward with these changes, I think we should see if we can find a way to avoid making temporary changes to validation code that are not desirable for the long term. Specifically:

- Having `interrupt_on_fatal_error` and `interrupt_on_condition` ChainStateManager options which are complicated (and very confusing even to me!)
- Giving ChainStateManager a mutable reference to the interrupt object instead of
...

💬 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.