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

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