Skip to content

[Draft] Fix flushing of rejects before disconnecting - #2953

Draft
Liquid369 wants to merge 4 commits into
PIVX-Project:masterfrom
Liquid369:2024_dash_3429
Draft

Liquid369 wants to merge 4 commits into
PIVX-Project:masterfrom
Liquid369:2024_dash_3429

Conversation

@Liquid369

Copy link
Copy Markdown
Member

This was only used in only one remaining place and only to ensure that
reject messages are sent before closing sockets. This is solved by the
previous commit now.
@Liquid369
Liquid369 force-pushed the 2024_dash_3429 branch 5 times, most recently from 410cfd0 to 9b32635 Compare January 14, 2025 21:28
@mkorovkin2

Copy link
Copy Markdown

NACK

Reviewed and tested merge result against current master.

Blockers:

  • DisconnectNodes neither advances it for connected peers nor preserves it after erase, causing an infinite loop/undefined behavior while holding cs_vNodes.
  • GDB attached to the exact test-created pivxd: its hot b-pivx-net thread was stopped in this loop while b-pivx-msghand waited on cs_vNodes; an iterator-only fix made the same cache workload finish in 14.86s.
  • The claimed reject-flush purpose is not implemented: reject state/consumer were restored, but nothing enqueues CBlockReject; this also partially reverses PIVX's deliberate BIP61 removal without rationale or tests.

Additional issues:

  • The 100ms linger busy-spins under cs_vNodes, and the partial one-second cleanup backport breaks disconnect, isolation, and reconnect timing.
  • Inactivity checks are duplicated; byte accounting is dead; teardown logs use global rather than peer masternode state; no regression tests were added.

Validation:

  • Exact PR: clean Linux build and full make check passed, but both 105-test default and 108-test extended runners failed during mandatory cache creation; fixed master completed it in 15.68s.
  • Disposable runtime correction: full rebuild and make check passed; default finished 101 pass/4 skip/0 fail, extended 104/4/0, and supplementary ZMQ passed. Three Linux-only bind cases remain uncovered due Docker storage EIO.
  • All 14 scripts excluded from the maintained runner were also executed separately: 2 passed and 12 failed from known legacy test drift.

Do not merge until iterator safety, bounded non-spinning cleanup, PIVX reject policy, and regression coverage are resolved. Lmk if there's anything I missed here as well.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants