💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1568710074)
After discussing offline with @fanquake, it doesn't make sense to let a single test keep us from upgrading our entire darwin toolchain infrastructure. I've removed the lief check for now until we can bump to >= 0.13.0.

💬 theuni commented on issue "Adding a minimally-patched Guix repo to the org":
(https://github.com/bitcoin/bitcoin/issues/25098#issuecomment-1568719209)
Reopening for https://github.com/bitcoin/bitcoin/pull/27676.

One of those commits introduces a new guix fork in order to [patch the LLVM build](https://github.com/theuni/guix/commits/fix-clang-includes). In this case the typical overrides don't work, and forking llvm as @dongcarl suggested here seems to be the most straightforward solution. I intend to upstream the changes eventually, but even if accepted that means a (potentially lengthy) delay and a time machine bump.

If there are no ob
...

⚠️ theuni reopened an issue: "Adding a minimally-patched Guix repo to the org"
(https://github.com/bitcoin/bitcoin/issues/25098)
As of e6a94d44469f90f4dc88a07a5a8587730811c705, we use Guix's upstream git repo on https://git.savannah.gnu.org/git/guix.git for our `guix time-machine`-powered pinning.

However, it does seem that we occasionally will encounter problems with Guix's upstream repo which are not easily fixed using the Guix package transformation flags.

In particular:

1. A test of `gnutls` failed because of a chronological error whereby a certificate used in a test expired. (#21203)
- We waited until G
...

💬 MarcoFalke commented on issue "Adding a minimally-patched Guix repo to the org":
(https://github.com/bitcoin/bitcoin/issues/25098#issuecomment-1568730528)
> even if accepted that means a (potentially lengthy) delay

Have you tried it? If this goes into upstream within a week or two and all what is left for us to bump the time machine, this doesn't seem like a reason to create the fork, only to remove it again in two weeks.

💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1568736418)
I refactored the code and updated the PR. The new code is split between two branches:
1) deniability-api - new API and implementation of core "deniabilization" functions. I'll open a PR in the main repo once I add RPC and unit tests.
2) deniability - the GUI portion of the Deniability dialog which uses the new deniability-api functions.

💬 MarcoFalke commented on pull request "[PoC] Structure aware fuzzing with libprotobuf-mutator":
(https://github.com/bitcoin/bitcoin/pull/26975#issuecomment-1568737329)
Did you find that this was useless in a benchmark, or did you just close for fun :smiling_face_with_tear: ?

💬 theuni commented on issue "Adding a minimally-patched Guix repo to the org":
(https://github.com/bitcoin/bitcoin/issues/25098#issuecomment-1568737780)
> > even if accepted that means a (potentially lengthy) delay
>
> Have you tried it? If this goes into upstream within a week or two and all what is left for us to bump the time machine, this doesn't seem like a reason to create the fork, only to remove it again in two weeks.

@fanquake mentioned that we're currently blocked from bumping our time machine for an unrelated reason, so I figured the fork would be necessary either way.

But no, I haven't yet submitted the patch upstream. I'm g
...

👋 TheCharlatan's pull request is ready for review: "kernel: Remove args, chainparams, chainparamsbase from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27576)

💬 mzumsande commented on pull request "Fix rpc_getblockfrompeer intermittency by introducing 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1568771908)
> np for me. @mzumsande do you want to go ahead with your fix? I can just relabel this PR to "Implement getblockfileinfo RPC command" and done.

yes, I'll open a PR with the one-line fix soon!

💬 hebasto commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1210571545)
> What's the reason for leaving certain mappings out of this list, i.e `#include <boost/operators.hpp>`?

To keep this PR in reviewable state. For example, a bunch of public Boost headers must be included explicitly and add to a linter:
- `boost/multi_index/identity.hpp`
- `boost/multi_index/indexed_by.hpp`
- `boost/multi_index/tag.hpp`
- `boost/tuple/tuple.hpp`

> I'm guessing it's because those same includes are recommended for non-test code, and mapping them to Boost Test here would j
...

💬 hebasto commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#issuecomment-1568778803)
Addressed @fanquake's https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1210341923.

💬 brunoerg commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1210580514)
If the purpose is maturing them, I think we could do:
```suggestion
self.generate(self.nodes[0], COINBASE_MATURITY) # mature coinbase UTXO used later
```

📝 hebasto opened a pull request: "Add public Boost headers explicitly"
(https://github.com/bitcoin/bitcoin/pull/27783)
To check symbols in the code base, run:
```
git grep boost::multi_index::identity
git grep boost::multi_index::indexed_by
git grep boost::multi_index::tag
git grep boost::make_tuple
```

Hoping on the absence of conflicts with top-prio PRs :)

💬 MarcoFalke commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210593227)
Why are they in the header when they can be in the cpp file?

💬 MarcoFalke commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1568815816)
Looks like the fuzz test needs an update?

```
82bb7831fa6052620998c7eef47e48ed594248a8 is the first bad commit
```

```
echo 'AQAAAAEAAAA='|base64 --decode > /tmp/a
FUZZ=wallet_notifications ./src/test/fuzz/fuzz /tmp/a
```

```
fuzz: wallet/test/fuzz/notifications.cpp:175: void wallet::(anonymous namespace)::wallet_notifications_fuzz_target(FuzzBufferType): Assertion `total_amount == bal_a + bal_b' failed.
==301525== ERROR: libFuzzer: deadly signal

🤔 stickies-v reviewed a pull request: "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN"
(https://github.com/bitcoin/bitcoin/pull/27777#pullrequestreview-1451472783)
ACK fa9c65a74cf18e9c75cd3472112d5197532ac2f2

I've not used podman and don't fully understand the need for fa123077bc3f39aa0969d883e2d799a054cd4543, so I'm hoping that other people do and can comment on that so I don't need to dive into it. Otherwise, happy to revisit this later.

💬 stickies-v commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210602495)
Okay, I can't comment on that. Since there is no mention of `podman` anywhere else in the codebase/documentation, I think this may make maintaining the CI more opaque, but perhaps that's an okay tradeoff.

💬 MarcoFalke commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210608472)
Yeah, I don't expect anyone to use `RESTART_CI_DOCKER_BEFORE_RUN` beside the persistent workers, so it is an undocumented setting. Anyone using it is expected to infer the documentation from the two lines of code.

💬 hebasto commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210610907)
```
$ git grep -l boost::multi_index::tag
src/node/miner.h
src/txmempool.h
src/txrequest.cpp
```

The symbol is used in the header. IWYU agree :)

💬 MarcoFalke commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210612717)
what is the point in adding it and using it over just `assert` or `Assert` or `BOOST_REQUIRE`?