💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1561872278)
this PR is ready for review!

did a couple of clean-up steps:
* removed the fuzz commit since coverage is now handled in #27549
* rebased on master
* updated the OP to reflect current status

👋 amitiuttarwar's pull request is ready for review: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213)

👍 pinheadmz approved a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1442687736)
re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65

Confirmed nits addressed since last ACK, minimal diff. All tests pass locally for me, CI failures are unrelated to code changes.

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRucOIACgkQ5+KYS2KJ
yTpqZg//YoQc3ivctelv3QQLCGcGruDO9twOQMm8TRx+8mZRtIV2UqPom2pwLjXM
BnZ
...

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204729225)
s/121/112/ ?

💬 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-1561897120)
Added a custom forked guix repo containing the patch we need for llvm's build.

I *believe* I've fixed authorization and it should just work(tm).

My local guix build works now, but I've reworked things so many times it would be helpful to get a confirmation.

Going to mark as ready for review since guix builds might actually work now.

👋 theuni's pull request is ready for review: "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM"
(https://github.com/bitcoin/bitcoin/pull/27676)

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204739973)
taproot coinbase outputs wen

LGTM

👍 instagibbs approved a pull request: "validate package transactions with their in-package ancestor sets"
(https://github.com/bitcoin/bitcoin/pull/26711#pullrequestreview-1442724105)
ACK https://github.com/bitcoin/bitcoin/pull/26711/commits/b234f993da3caa16f7716948fbcd16ab1a500180

modulo magic numbers in tests (sorry). Ran the fuzzer for a while again, no issues this time.

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204742880)
diamond keeps going "out of sync", maybe just have variable names that people can find?

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1204795533)
I think "on local ports" is unnecessarily restrictive/scary. I think it's still completely fine to whitelist remote addresses, you mostly just want to avoid ranges, right?

💬 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-1562038331)
See relevant #25098. If we go forward with this we'll want the guix fork as part of our github org rather than my personal repo.

🤔 furszy reviewed a pull request: "wallet, bench: Move commonly used functions to their own file and fix a bug"
(https://github.com/bitcoin/bitcoin/pull/27666#pullrequestreview-1442882343)
Code ACK, small last topic:

Seems that `BenchLoadWallet` and `BenchUnloadWallet` are similar to `TestLoadWallet` and `TestUnloadWallet` from `wallet_tests.cpp` . The only diff seems to be in the db.

Maybe instead of creating a new file, we could move the bench functions to `wallet/test/util.h` and `wallet/test/util.cpp` so they are used equally for tests and benchmarks?

💬 furszy commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1204852855)
tiny nit: could wrap the functions around a "namespace wallet {}" instead.

💬 theStack commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1204878968)
Should probably also mention here that the inputs are popped from the stack? E.g. something like
`pop two top stack items and push 1 if they are equal, 0 otherwise`

💬 theStack commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1204876854)
```suggestion
OP_IFDUP = 0x73, // if top stack value is true, duplicate it
```

💬 theuni commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204902245)
Sure. I only asked because it looked like it was possible in this case. But this can be done after-the-fact if we decide that's what we want.

Longer-term, we'd want to detach from our internal structures to avoid changing the abi every time we change something internally. So I don't imagine `CBlockIndex` ever being part of a public api. Maybe a trivial wrapper or something, but not CBlockIndex itself. (It's also worth keeping that in mind at this stage because that external-to-internal-compon
...

📝 russeree opened a pull request: "Use 'byte'/'bytes' for bech32(m) validation error message - 27727 follow up."
(https://github.com/bitcoin/bitcoin/pull/27747)
This PR rectifies a linguistic inconsistency found in merged PR [27727](https://github.com/bitcoin/bitcoin/pull/27727). It addresses the improper usage of the term 'byte' in error reports. As it stands, PR [27727](https://github.com/bitcoin/bitcoin/pull/27727) exclusively utilizes 'byte' in error messages, regardless of the context, as demonstrated below:

Currently: ```Invalid Bech32 v0 address program size (16 byte), per BIP141```

This modification enhances the accuracy of error reporting
...

💬 ChrisCho-H commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1562164731)
Thanks! I've fixed it.

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204928358)
not anymore. Re-worked the first commit so it only touches the `wallet_balance.cpp` bench file and nothing else.

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204932281)
Ended up pushing a simpler approach. Focused on correcting `wallet_balance.cpp` only.

In the future, we could generate a fake chain instead of creating a "real" one. Similar to how it's done
in the `wallet_create_tx.cpp` benchmark.