Skip to content

txpool: don't queue dup txs for relay in strand [beta] - #433

Open
j-berman wants to merge 1 commit into
seraphis-migration:fcmp++-beta-stressnetfrom
j-berman:no-dup-strand-relays-beta
Open

txpool: don't queue dup txs for relay in strand [beta]#433
j-berman wants to merge 1 commit into
seraphis-migration:fcmp++-beta-stressnetfrom
j-berman:no-dup-strand-relays-beta

Conversation

@j-berman

@j-berman j-berman commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Note this PR is in collaboration with @selsta.

While running #420 on this modified branch, @Rucknium still observed double spend errors after some time. The logs indicate that the strand relaying txs is getting backed up, and duplicate txs get queued for relay in the strand as a result. #420 exacerbates the issue because local txs can get piled up in the relay strand, continuously getting re-dispatched to the strand before the strand executes relaying even one of them (once the tx gets relayed in the strand, it would then prevent the re-relay loop from re-queuing for relay).

This PR ensures that duplicate txs won't get queued for relay in the strand.


I have a lingering thought that it may be the case that this PR is unnecessary once the strand getting backed up is solved [UPDATE 8/4/26: this PR does still appear necessary, see this comment]. I'm thinking I will request @Rucknium run this patch with more thorough logging + @vtnerd's patch once ready, so we can see if A) the double spend issue is solved for good, and B) if this PR is actually necessary or not (i.e. if txs are piling up in the strand even after the strand bottleneck is solved).

Here's a helpful log snippet from @Rucknium for reference:
bitmonero.log_2026-06-29-23-26-56.tar.gz

Note how Going to re-relay local tx <60df75853588a435a59b13a46b3964829606ccb42b938d2c0bb44aaccfc2bc04> appears in that log file 69 times in relatively short intervals (and is also marked as a double spend in there).

@j-berman j-berman mentioned this pull request Jul 13, 2026
7 tasks
@vtnerd

vtnerd commented Jul 14, 2026

Copy link
Copy Markdown

#435

@vtnerd

vtnerd commented Jul 14, 2026

Copy link
Copy Markdown

I don't think this completely solves the potential issue. The P2P-TCP server has a send buffer, whose latency is determined by the reader on the remote side, AND potential rate-limiting configuration by the user. I don't see how we can remove that buffer, or config option.

As a result, D++ txes could be stuck in the send buffer a bit, but be marked as dequeued by this code. Additionally, the latency in D++ code should be really short with #435, provided we don't have any more blocking code in the TCP server.

If we want to track status in the TCP server, thats likely a larger overhaul.

@j-berman

j-berman commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

The issue this PR is attempting to work around is mainly that on_transactions_relayed in dandelionpp_notify's () operator can take a while to get called, because the zone strand is backed up. on_transactions_relayed is what prevents the relay_txpool_transactions re-relay loop from trying to re-queue a tx for relay potentially every 1s once the tx is ready for re-relay.

Looking at #435, the zone strand may be backed up because wait_consume is waiting for the send buffer to fall back below the soft limit. And #435 would prevent that sit-and-wait by just dropping the connection once the soft limit is tripped.

So, with #435, I'm thinking we may just see connection drops, rather than a backed up zone strand leading to enqueued duplicate relays. And that would render this PR not necessary.


FWIW I see your point that it's still possible for duplicate txs to enter the send buffer queue even with this PR. That would be ok, since this PR is just aiming to work around the issue of on_transactions_relayed not getting called before the next re-relay tick, rather than a duplicate tx actually sending over the wire / in the send buffer queue.

@j-berman

j-berman commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

It looks like the node is still getting backed up waiting for blocks/txs to verify. Since on_transactions_relayed blocks waiting on those actions, it seems to be a leading cause of the relay strand getting backed up as well.

@Rucknium's logs indicate the node does benefit from this change since the strand is still getting backed up (the log files are too large to share in this comment).

As such I think this PR is justified and worth including both here and upstream, and ultimately #440 would potentially be most helpful for relieving the backlog in the strand (since it avoids grabbing a lock to verify pool txs).

@j-berman
j-berman marked this pull request as ready for review August 4, 2026 17:08
@vtnerd

vtnerd commented Aug 4, 2026

Copy link
Copy Markdown

@j-berman you mean on_transactions_relayed gets blocked via mutex by the block/tx insertion?

@j-berman

j-berman commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants