💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282837698)
added

💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282850596)
Edited the comments.

Without delays and filtering right before sending, I figured you could query whether `tx_real` has arrived in the node's mempool yet by sending a fake orphan that spends from it (if the node requests `tx_real` they don't have it yet, if they don't request it then it's already in mempool/seen). Hence "reveal." But you can only know whether `tx_real` arrived in the last ~2-4 seconds.

💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282862310)
True this could be combined. The main idea for this test is that this transaction's failure propagates all the way to the grandchild.

💬 fanquake commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1663715708)
> Created in the context of https://github.com/bitcoin/bitcoin/issues/28109, but I don't think it is enough to actually fix this problem.

It's a step in the right direction, we're now failing on `45934` sections, compared to `46006` in #28109. However we're still a long way north of the limit, which as I understand it, is `32768`.

👍 stickies-v approved a pull request: "net, refactor: remove unneeded exports, use helpers over low-level code, use optional"
(https://github.com/bitcoin/bitcoin/pull/28078#pullrequestreview-1560767388)
ACK 4ecfd3eaf434d868455466e001adae4b68515ab8

That said:
1. I was ~0 on the `[[nodiscard]]` changes before, but am now in favour of removing all the ones in this PR. The only place where it made sense, imo, was in 5ba73cd0ee1e661ec4d041ac8ae7a60cfd31f0c0 for `[[nodiscard]] static bool GetLocal(CService& addr, const CNode& peer)` where it's dangerous to use the out-parameter without checking the return value. Now, in all cases, the functions are entirely self explanatory and I don't really see
...

💬 feed3r commented on issue "Master doesn't compile on MacOS X 10.13 High Sierra":
(https://github.com/bitcoin/bitcoin/issues/28206#issuecomment-1663735680)
Thank you all guys, yes this is not a bug and I was only looking for any common experience.
I had a solution anyway and will go with a release checking the compatibility in the release notes.
Thank you all again!

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1283012394)
Note to self, this should be 1023

🤔 glozow reviewed a pull request: "refactor: Make more transaction size variables signed"
(https://github.com/bitcoin/bitcoin/pull/28059#pullrequestreview-1560810508)
ACK 92de74ef181b42d774bc6b12329bc0c27caf0081

🚀 glozow merged a pull request: "refactor: Make more transaction size variables signed"
(https://github.com/bitcoin/bitcoin/pull/28059)

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283023840)
Yeah, unless any objections this would have my preference, less side effects and easier to understand imo:

<details>
<summary>git diff on 26fe348e6e05d9b50614fdcba9cfb7a711ae86dc</summary>

```diff
diff --git a/src/dbwrapper.h b/src/dbwrapper.h
index 54c4685c48..48a6c9af00 100644
--- a/src/dbwrapper.h
+++ b/src/dbwrapper.h
@@ -92,9 +92,6 @@ private:
const CDBWrapper &parent;
leveldb::WriteBatch batch;

- DataStream ssKey{};
- CDataStream ssValue;
-
size_t
...

📝 darosior opened a pull request: "fuzz: a target for the block index database"
(https://github.com/bitcoin/bitcoin/pull/28209)
This introduces a small fuzz target for `CBlockTreeDB` which asserts a few invariants by using an in-memory LevelDb.

💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283055797)
Not sure about making this a static var. Now the allocations survive beyond the lifetime of the `CDBBatch` objects and I am not sure if this is entirely safe.

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1283068149)
Updated. This also changes the initial value for `m_last_inv_sequence` to `1` instead of `0`. (Note that I reversed the order of the `if .. else` in `info_for_relay` so `GetSeq() < last` becomes `! (GetSeq() <= last)` which is `last < GetSeq()`)

💬 MarcoFalke commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1283055992)
not sure about adding test-only code to "real" code. What about moving this to the only fuzz test that needs it?

👍 MarcoFalke approved a pull request: "fuzz: a target for the block index database"
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-1560871534)
Concept ACK, left two nits

💬 MarcoFalke commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1283056551)
nit: remove newline?

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1663824558)
Update for glozow's review; fixes off-by-one error and adds some glozow's test coverage of reorg behaviour

💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1283080177)
Sure

💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1283080569)
Oh, i forgot to push my last change, this include shouldn't be here (that's why i had separated it).

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283080816)
Good point. Okay, let's mark as resolved.