💬 fanquake commented on pull request "Use hardened runtime on macOS release builds.":
(https://github.com/bitcoin/bitcoin/pull/29127#issuecomment-1866088324)
> The release maintainer, or any authorized Apple Developer, will need to run xcrun notarytool

How does someone do this on Linux, without macOS hardware/Xcode?

💬 glozow commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1866099123)
Agree with the intent of avoiding legal problems, but NACK on adding this text. Unless we have some kind of legal guidance saying this text would protect us beyond what our existing license-related docs say, I don't see any reason to discourage specific tools in the contributing guidelines. I agree with the above that trying to speculate/innovate can do more harm than good.

I think we should close this for now and reconsider if/when a a lawyer advises us to do something like this.

💬 hebasto commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1866125765)
> Since it is always part of the internal library, the `HAVE_CONSENSUS_LIB` define used by the tests is essentially
> meaningless, since the module is always available.

It seems worth noting that we do not use the `HAVE_CONSENSUS_LIB` macro in `/src/test/fuzz/script_bitcoin_consensus.cpp` in the master branch.

💬 kristapsk commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1866131660)
IMHO it would been simpler to just merge such trivial changes, but ok.

💬 hebasto commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1866158665)
Concept ACK on moving the `script/bitcoinconsensus.cpp` out from the internal `libbitcoin_consensus.a` library.

@theuni

> I think another way of looking at this is that `script/bitcoinconsensus.cpp` is moving out into its own convenience lib, it just happens to be a single file. We could imagine if the api were written in multiple translation units, we'd actually want to build that convenience lib.

That is very similar to what I suggested in https://github.com/bitcoin/bitcoin/pull/2499
...

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1866160684)
> Yes, that's what the compiler error says, but that function doesn't seem to be in that namespace. Its other callers don't qualify it, and if I do qualify it, the build fails for me.

Seems that the function was moved to the `util` namespace on #28075.
Try cleaning up your project and rebasing it on master. (git clean -fdx && git pull --rebase origin master && make).
Will need to do some modifications here. `LockDirectory()` does not return a boolean anymore.

📝 brunoerg opened a pull request: "wallet, rpc: add BIP44 `account` in `createwallet` "
(https://github.com/bitcoin/bitcoin/pull/29129)
This PR adds an `account` parameter in `createwallet` RPC. It's the
BIP44 account that will be used in `SetupDescriptorScriptPubKeyMans`
to fetch the descriptors from the external signer.

👋 fanquake's pull request is ready for review: "build: switch to using LLVM 17.x for macOS builds"
(https://github.com/bitcoin/bitcoin/pull/28880)

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866292269)
Guix build (x86_64 & aarch64):
```bash
65981453fcb83338ed76435dc8fed06e9903441731b461e4f2c8a04ad102043c guix-build-b335710782c2/output/aarch64-linux-gnu/SHA256SUMS.part
3127d18bc55a7a207eb7e1252feaf24f767d5983e5d064323671139659c4f3d7 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu-debug.tar.gz
0a67737c010e742249f07dd3ca3ae289df02edf2dedd8d6a5a07b291921bab47 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu.tar.g
...

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866295932)
This is now reproducible, and reviewable.

👍 BrandonOdiwuor approved a pull request: "wallettool: Always be able to dump a wallet's database"
(https://github.com/bitcoin/bitcoin/pull/29117#pullrequestreview-1793028431)
Code Review ACK d83bea42d1f0ffb0899a6de3556c489543468995

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432797027)
Rule 5 is not accurately checked here.
IMO its better to remove it completely and depend on `ApplyV3Rules` for an accurate child size limit check.
The method might even be removed completely in the future cc #28345

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432787931)
iwyu
```suggestion
#include <set>
```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432801208)
We can just return early if all the transactions are not v3
```suggestion
const auto any_v3 = std::any_of(package.begin(), package.end(), [](const auto& tx){ return tx->nVersion == 3;});
if (!any_v3) { return v3_ancestor_sets;}
```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432807598)
Nit: I find `tx_iter` more readable than `package_iter`, since its a transaction iterator not package iterator
```suggestion
for (auto tx_iter = package.rbegin(); tx_iter != package.rend(); ++tx_iter) {

```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432824698)
We can just skip if the ancestor has the transaction in it's descendant set?
```suggestion
if (ancestors_descendant_set.count((*tx_iter)->GetHash()) == 0 ) {
ancestors_descendant_set.insert(my_descendant_set.cbegin(), my_descendant_set.cend());
```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432811937)
Nit: IMO its more readable to define an intermediate txid variable here also just like its done in the for loop above
```suggestion
const Txid& my_txid{(*package_iter)->GetHash()};
if ((*package_iter)->nVersion == 3) {
```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434138603)
why increment `num_prechecks_passed` here ?

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434137263)
After restarting here, we are restarting at the beginning of the test again with extra args
Dev notes says
> [Avoid stop-starting the nodes multiple times during the test if possible. A stop-start takes several seconds, so doing it several times blows up the runtime of the test.](https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines)

The wrapper can accept the extra args as argument and restart at the beginning only ?
<details>

```python
def cleanup(
...

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434122383)
`CheckMempoolV3Invariants` docstring says we would verify
>any non-v3 tx must only have non-v3 ancestors

We are only getting parents and checking the first parent?