- User Since
- Apr 14 2022, 17:59 (5 w, 4 d)
- Slack Username
I gave my last comment some more thought and came to this conclusion: Even if my assumptions are correct in practice,
it's better (from a user UX perspective) to deploy with thresholds a bit higher than expected. Consider these scenarios:
I should also mention it's my expectation that confidence flips are not uniformly distributed, but more likely to occur earlier in the voting process. While it's possible to reach 127 confidence and reset twice, then hitting the low-end threshold, I believe this case to be rare.
Sat, May 21
- Rebase on changes in D11502
- Remove TestingSetup. It's not needed.
- Fix approach so that records will go stale for all confidence levels
- Add way more test coverage
- Tighten threshold for low-confidence stalls
Fri, May 20
Added finalization sanity check
Thu, May 19
This was only a concern in relation to changes I suggested in D11474. Since my approach on that is changing, this patch isn't relevant anymore.
Wed, May 18
Remove unnecesarily complex logic from isStale(). The banning circumstance isn't possible since my understanding was incorrect.
Add requested comments
Tue, May 17
- Add more test coverage
- Drop the stale vote in addition to reporting the VoteStatus
- Always check shouldPoll() in getInvsForNextPoll()
- Make the isStale() check more robust so that votes are only dropped when inflight polls have finished. Otherwise it's possible to ban peers that respond after the record has been dropped.
flagged -> marked
- Added some missing test cases
- Added requested log
- Make threshold unsigned
These timeouts are a symptom that the default is too long. They make local testing painful when debugging tests. We should consider lowering the default timeout if not too many tests are impacted, but that will be a separate change.
Mon, May 16
Extract some parts of this diff into D11474. The design is moving away from complicating the vote rejection logic in favor of letting the network layer request inventories as needed.
I think I came up with a better approach.
I compared versions with Fabien and the version I'm on is older. Installing flake8 and autopep8 via pip3 instead of apt gives the newer versions which give clean linting for this file.
Sun, May 15
Sat, May 14
Fix the approach so that instead of stopping avalanche on the inconclusive item, we continue
with a reject vote. This way, we retain network consistency but are still able to tip the
network to finalize a rejection and not let a vote dangle.
Fri, May 13
Thu, May 12
Generally true throughout this diff: the naming is too dependent on the underlying proofs database being a radix tree. A simple example at the peer manager interface is getProofRadixTree(). Does the caller care that the DB is a radix tree? If we change to some other COW DB, we'll end up with a lot of renaming to do.
Wed, May 11
Tue, May 10
Mon, May 9
Fri, May 6
Maybe mention in the summary that changes related to g_wtxid_relay_peers are not present because these are for segwit.
Attempt to handle unexpected DUPLICATED case gracefully
Thu, May 5
A quick mention of what you intend to do with this would be helpful since it's not currently used except for its own test.
Wed, May 4
Fri, Apr 29
Thu, Apr 28
This single flag is trying to do two jobs, so it ends up being a tri-state switch, which I doubt is expected by the user:
- Unset - non-avalanche connections do not bypass, but avalanche connections do
- True - all connections bypass
- False - no connections bypass