💬 sipa commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807326135)
@theStack Yeah, I think that'd be closer to what one would expect such a number to mean.

💬 sipa commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807345702)
@theStack (deleted my previous comment, I had missed a lot).

`RecursiveDynamicUsage` for `CTransactionRef` (which is a `std::shared_ptr<CTransaction>`) should already account for both the memory allocated in the shared pointer as well as the memory allocated by the transaction. See `src/core_memusage.h`:

```c++
template<typename X>
static inline size_t RecursiveDynamicUsage(const std::shared_ptr<X>& p) {
return p ? memusage::DynamicUsage(p) + RecursiveDynamicUsage(*p) : 0;
}

```
...

💬 theStack commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807405302)
@sipa: Ah, very interesting, have to admit I completely overlooked `RecursiveDynamicUsage(const std::shared_ptr<X>& p)`.

> Is it possible that you just don't see the added field cause changes because either it's fitting in earlier padding space, or because the rounding up effect of `malloc()` hides the difference?

Yes, it was the latter. The added field did lead to a size increase of `CTransaction` (from 120 to 120 bytes), but `MallocUsage` indeed yields the same value for both of these [s
...

💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1390623656)
Thanks!
I think you should actually use `net` by making all callers from `p2p.py` pass it to `EncryptedP2PState`, the default value of 'regtest' should be dropped in my opinion. You can verify that it works when `p2p_dos_header_tree.py --transport` doesn't fail anymore.

Could also change all existing imports for MAGIC_BYTES (there are a few in other tests) to avoid indirect imports.

💬 Sjors commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#issuecomment-1807534141)
Concept ACK

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1807535377)
> Should we spend a bit more time discussing the public API? I.e. what to name `getxpub` method and how we would like to evolve in the future?

I'm keeping #22341 rebased on top of this.

💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1390663584)
Might indeed just give the wallet some keys, so later tests can have transactions and rescans.

Which formatter?

💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1390665847)
Dropped

💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#issuecomment-1807555704)
Pushed some cleanups.

💬 Sjors commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28851#issuecomment-1807573920)
Getting the same hashes as @hebasto. Briefly tested that the binary still works on Intel macOS 13.6

💬 maflcko commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1390771582)
Could add a comment why this is needed?

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1390822510)
Not sure why that'd be difficult. In C++, it is possible to forward declare a struct:

```diff
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 7c905ca4f4..199fe948a3 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -22,6 +22,9 @@
#include <stdint.h>

class CBlockIndex;
+class CTxMemPoolEntry;
+
+using CTxMemPoolEntryRef = std::reference_wrapper<const CTxMemPoolEntry>;

struct LockPoints {
// Will be set to the blockc
...

💬 fanquake commented on pull request "guix: switch to 6.1 kernel headers over 5.15":
(https://github.com/bitcoin/bitcoin/pull/28786#issuecomment-1807787620)
> Sent a patch upstream, https://lists.gnu.org/archive/html/guix-patches/2023-11/msg00362.html, to see if we can get some unversioned pointers to stable/longterm added.

No traction on this yet, going to move forward here.

> Yet boost doesn't make the same compatibility guarantees, and needs to be checked.

I haven't seen anything that would cause issues, let me know / open an issue if you see otherwise.

💬 dergoegge commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1390843395)
I think this runs into the same timeouts as the `mocked_descriptor_parse` target. We should probably wait for #28832 to be merged so `HasDeepDerivPath` can be used here as well.

🚀 fanquake merged a pull request: "guix: switch to 6.1 kernel headers over 5.15"
(https://github.com/bitcoin/bitcoin/pull/28786)

👋 fanquake's pull request is ready for review: "guix: update time-machine"
(https://github.com/bitcoin/bitcoin/pull/28580)

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1807801984)
A fresh one: [clusterfuzz-testcase-load_external_block_file-5183508842414080.bin.txt](https://github.com/bitcoin/bitcoin/files/13333454/clusterfuzz-testcase-load_external_block_file-5183508842414080.bin.txt)

💬 fanquake commented on pull request "guix: update time-machine":
(https://github.com/bitcoin/bitcoin/pull/28580#issuecomment-1807805694)
LLVM 16 & 17 are now available. This is ready for review. This bump also contains the recently released [Mes 0.25](https://www.mail-archive.com/info-gnu@gnu.org/msg03232.html), which should improve the bootstrapping experience for RISC-V. cc @laanwj who upstreamed a bunch of the changes there.

💬 fanquake commented on pull request "doc: update docs for `CHECK_ATOMIC` macro":
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1807828903)
See also the CMake implementation of this check: https://github.com/hebasto/bitcoin/pull/50.

🚀 fanquake merged a pull request: "doc: update docs for `CHECK_ATOMIC` macro"
(https://github.com/bitcoin/bitcoin/pull/28777)