Page MenuHomePhabricator

[avalanche] Let the PeerManager use the ProofPool
AbandonedPublic

Authored by Fabien on Nov 11 2021, 12:43.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Summary

This diff replaces the utxos structure in the peer manager with the ProofPool.

This pool is configured with a limited depth of a single proof per utxo, and the proof utxos attachment is made so that there is no change in behavior.

The utxos attachement call is moved from createPeer() method to the registerProof() as it is about proof and not peer.

Ref T1854.

Depends on D10453 and D10459.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_peermanager_utxostore
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17241
Build 34313: Build Difflint-circular-dependencies · build-without-wallet · build-clang-tidy · build-diff · build-clang · build-debug
Build 34312: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Nov 11 2021, 12:43

Tail of the build log:

[392/451] bitcoin: testing scheduler_tests
[393/451] Running utility command for check-bitcoin-scheduler_tests
[394/451] bitcoin: testing cuckoocache_tests
[395/451] bitcoin: testing merkleblock_tests
[396/451] Running utility command for check-bitcoin-cuckoocache_tests
[397/451] bitcoin: testing bip32_tests
[398/451] Running utility command for check-bitcoin-merkleblock_tests
[399/451] bitcoin: testing sync_tests
[400/451] Running utility command for check-bitcoin-bip32_tests
[401/451] Running utility command for check-bitcoin-sync_tests
[402/451] bitcoin: testing torcontrol_tests
[403/451] Running utility command for check-bitcoin-torcontrol_tests
[404/451] bitcoin: testing settings_tests
[405/451] Running utility command for check-bitcoin-settings_tests
[406/451] bitcoin: testing streams_tests
[407/451] bitcoin: testing timedata_tests
[408/451] Running utility command for check-bitcoin-streams_tests
[409/451] Running utility command for check-bitcoin-timedata_tests
[410/451] bitcoin: testing validation_flush_tests
[411/451] Running utility command for check-bitcoin-validation_flush_tests
[412/451] bitcoin: testing op_reversebytes_tests
[413/451] bitcoin: testing compilerbug_tests
[414/451] Running utility command for check-bitcoin-op_reversebytes_tests
[415/451] bitcoin: testing blockcheck_tests
[416/451] Running utility command for check-bitcoin-compilerbug_tests
[417/451] Running utility command for check-bitcoin-blockcheck_tests
[418/451] bitcoin: testing checkpoints_tests
[419/451] bitcoin: testing serialize_tests
[420/451] Running utility command for check-bitcoin-serialize_tests
[421/451] Running utility command for check-bitcoin-checkpoints_tests
[422/451] bitcoin: testing script_standard_tests
[423/451] bitcoin: testing radix_tests
[424/451] bitcoin: testing validationinterface_tests
[425/451] Running utility command for check-bitcoin-script_standard_tests
[426/451] bitcoin: testing schnorr_tests
[427/451] Running utility command for check-bitcoin-validationinterface_tests
[428/451] Running utility command for check-bitcoin-radix_tests
[429/451] bitcoin: testing cashaddr_tests
[430/451] bitcoin: testing script_tests
[431/451] bitcoin: testing util_tests
[432/451] bitcoin: testing transaction_tests
[433/451] bitcoin: testing crypto_tests
[434/451] Running utility command for check-bitcoin-schnorr_tests
[435/451] Running utility command for check-bitcoin-cashaddr_tests
[436/451] bitcoin: testing monolith_opcodes_tests
[437/451] Running utility command for check-bitcoin-transaction_tests
[438/451] Running utility command for check-bitcoin-util_tests
[439/451] Running utility command for check-bitcoin-crypto_tests
[440/451] bitcoin: testing versionbits_tests
[441/451] Running utility command for check-bitcoin-script_tests
[442/451] Running utility command for check-bitcoin-monolith_opcodes_tests
[443/451] Running utility command for check-bitcoin-versionbits_tests
[444/451] bitcoin: testing intmath_tests
[445/451] Running utility command for check-bitcoin-intmath_tests
[446/451] bitcoin: testing coinselector_tests
[447/451] Running utility command for check-bitcoin-coinselector_tests
[448/451] bitcoin: testing coins_tests
[449/451] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

Rebase, add missing dependency. This will also restart the test that failed for an unrelated reason.

Fabien planned changes to this revision.Nov 18 2021, 12:15
Fabien retitled this revision from [avalanche] Let the PeerManager use the UtxoStore to [avalanche] Let the PeerManager use the ProofPool.Nov 18 2021, 12:40
Fabien edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Nov 24 2021, 01:48
deadalnix added a subscriber: deadalnix.

This is strictly more code and complexity to do the same thing, slower. I don't see what this brings to the table. The diffs description says what the cod does, which i already knew looking at the code, but in the absenc of a justification for this, I see no reason to even consider moving in that direction. This is strictly worse on all fronts.

src/avalanche/peermanager.cpp
491 ↗(On Diff #30910)

How is this better than what's on the left? It's more code, it's almost certainly slower, it's obvious looking at both sie by side that this is not moving in the right direction.

src/avalanche/peermanager.h
179 ↗(On Diff #30910)

Even better, this adds a ton of complexity under the hood and uses none of it. Now instead of having proofs, we have a map of st of on element. Which means we multiplied the failure modes. Now we can have empty sets for instance.

This revision now requires changes to proceed.Nov 24 2021, 01:48
Fabien requested review of this revision.Feb 9 2022, 13:28