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

📝 LarryRuane opened a pull request: "bugfix: account for system-allocated memory in pool resource"
(https://github.com/bitcoin/bitcoin/pull/27748)
Follow-on to PR #25325, "Add pool based memory resource"

The `DynamicUsage()` function for the version of `unordered_map` that uses the `PoolAllocator` returns a close approximation of the amount of physical memory used by the map. (This is the map used for the dbcache.) It accounts for several of the allocator's internal data structures, such as the memory chunks, the freelist, and the unordered_map's bucket list. But it doesn't account for memory that had to be obtained from the system allo
...

💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204957663)
This test may be somewhat fragile; it depends on `MallocUsage(8) == 32`. Maybe the test condition here should be `8 <= resource.SystemBytes()`, but I think that doesn't test enough.

💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204956602)
Copied this from https://github.com/bitcoin/bitcoin/blob/a13f3746dccd9c4ec16d6bfe9b33ebd26e3238e1/src/memusage.h#L50 because including that file would create a circular dependency. Is there a better way to do this?

💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1562231512)
> You can leave this open and rebase it onto the main repo PR, just mention it in the description.

All right, I'll start working on this refactor.
Btw, do we have some docs on how to setup git locally so I can have dependent changes across the "bitcoin" and the "gui" repos? So far I was moving changes via patches but that doesn't seem too convenient.