Page MenuHomePhabricator
Differential D9440

[avalanche] Move proof verification to processor
ClosedPublic
Actions

Authored by Fabien on Apr 21 2021, 07:52.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABC35e52efad474: [avalanche] Move proof verification to processor
Summary

This is an attempt to fix the processor initialization by adding a
factory method to build a Processor which will check the input arguments
consistency and build a Processor instance accordingly, or return a null
ptr and an error string. The Processor is only built if the arguments
are right. A few additional checks are added for the private keys.

This is based on D9368.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
processor_factory
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/avalanche/test/processor_tests.cpp:422CPPCHECKcontainerOutOfBounds
Errorsrc/avalanche/test/processor_tests.cpp:423CPPCHECKcontainerOutOfBounds
Errorsrc/avalanche/test/processor_tests.cpp:443CPPCHECKcontainerOutOfBounds
Errorsrc/avalanche/test/processor_tests.cpp:444CPPCHECKcontainerOutOfBounds
Unit
No Test Coverage
Build Status
Buildable 15556
Build 31032: Build Diffbuild-without-wallet · build-clang-tidy · lint-circular-dependencies · build-diff · build-clang · build-debug
Build 31031: arc lint + arc unit

Event Timeline

Fabien created this revision.Apr 21 2021, 07:52
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 21 2021, 07:52
Harbormaster completed remote builds in B15552: Diff 28240.Apr 21 2021, 07:52
Fabien requested review of this revision.Apr 21 2021, 07:52
Fabien added a comment.Apr 21 2021, 07:58

Note to reviewers: the lint errors are false positive from a cppcheck bug introduced in a recent version. There is no such issue if you run arc lint against this patch on Debian.

deadalnix requested changes to this revision.Apr 21 2021, 12:08
deadalnix added a subscriber: deadalnix.

Ok, now this is looking more like something that make sense. There is this encoding/decoding of the session key that needs to be addressed, but beside that, it's all good.

src/avalanche/processor.cpp
265 ↗(On Diff #28240)

Why are you encoding the session key and then decoding it? That makes no sense.

This revision now requires changes to proceed.Apr 21 2021, 12:08
Fabien updated this revision to Diff 28244.Apr 21 2021, 12:46

Don't encode/decode the session key

Harbormaster completed remote builds in B15556: Diff 28244.Apr 21 2021, 12:46
deadalnix requested changes to this revision.Apr 21 2021, 18:26
deadalnix added inline comments.
src/avalanche/processor.cpp
172

You are taking ownership of this key. So that it by value not ref. It leaves the caller with the option to move or copy depending on need.

This revision now requires changes to proceed.Apr 21 2021, 18:26
Fabien updated this revision to Diff 28246.Apr 21 2021, 20:08

Pass the session key by value and move it

Harbormaster completed remote builds in B15558: Diff 28246.Apr 21 2021, 20:08
teamcity added a comment.Apr 21 2021, 20:18
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

rpc_psbt.py                                      | ✓ Passed  | 19 s
rpc_rawtransaction.py                            | ✓ Passed  | 38 s
rpc_scantxoutset.py                              | ✓ Passed  | 3 s
rpc_setban.py                                    | ✓ Passed  | 2 s
rpc_signmessage.py                               | ✓ Passed  | 1 s
rpc_signrawtransaction.py                        | ✓ Passed  | 1 s
rpc_txoutproof.py                                | ✓ Passed  | 2 s
rpc_uptime.py                                    | ✓ Passed  | 1 s
rpc_users.py                                     | ✓ Passed  | 5 s
rpc_whitelist.py                                 | ✓ Passed  | 1 s
tool_wallet.py                                   | ✓ Passed  | 4 s
wallet_abandonconflict.py                        | ✓ Passed  | 5 s
wallet_address_types.py                          | ✓ Passed  | 15 s
wallet_avoidreuse.py                             | ✓ Passed  | 3 s
wallet_backup.py                                 | ✓ Passed  | 30 s
wallet_balance.py                                | ✓ Passed  | 16 s
wallet_basic.py                                  | ✓ Passed  | 32 s
wallet_coinbase_category.py                      | ✓ Passed  | 1 s
wallet_create_tx.py                              | ✓ Passed  | 5 s
wallet_createwallet.py                           | ✓ Passed  | 2 s
wallet_createwallet.py --usecli                  | ✓ Passed  | 3 s
wallet_descriptor.py                             | ✓ Passed  | 6 s
wallet_disable.py                                | ✓ Passed  | 1 s
wallet_dump.py                                   | ✓ Passed  | 5 s
wallet_encryption.py                             | ✓ Passed  | 5 s
wallet_groups.py                                 | ✓ Passed  | 52 s
wallet_hd.py                                     | ✓ Passed  | 5 s
wallet_import_rescan.py                          | ✓ Passed  | 5 s
wallet_import_with_label.py                      | ✓ Passed  | 1 s
wallet_importdescriptors.py                      | ✓ Passed  | 5 s
wallet_importmulti.py                            | ✓ Passed  | 3 s
wallet_importprunedfunds.py                      | ✓ Passed  | 2 s
wallet_keypool.py                                | ✓ Passed  | 3 s
wallet_keypool_topup.py                          | ✓ Passed  | 3 s
wallet_labels.py                                 | ✓ Passed  | 2 s
wallet_listreceivedby.py                         | ✓ Passed  | 15 s
wallet_listsinceblock.py                         | ✓ Passed  | 2 s
wallet_listtransactions.py                       | ✓ Passed  | 10 s
wallet_multiwallet.py                            | ✓ Passed  | 14 s
wallet_multiwallet.py --usecli                   | ✓ Passed  | 16 s
wallet_reorgsrestore.py                          | ✓ Passed  | 3 s
wallet_resendwallettransactions.py               | ✓ Passed  | 5 s
wallet_txn_clone.py                              | ✓ Passed  | 1 s
wallet_txn_clone.py --mineblock                  | ✓ Passed  | 3 s
wallet_txn_doublespend.py                        | ✓ Passed  | 2 s
wallet_txn_doublespend.py --mineblock            | ✓ Passed  | 3 s
wallet_watchonly.py                              | ✓ Passed  | 1 s
wallet_watchonly.py --usecli                     | ✓ Passed  | 1 s
wallet_zapwallettxes.py                          | ✓ Passed  | 4 s

ALL                                              | ✓ Passed  | 911 s (accumulated) 
Runtime: 182 s

----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
Fabien added a comment.Apr 21 2021, 20:47

The build failure is an unrelated leveldb spurious failure

Fabien added a child revision: D9422: [avalanche] Don't turn on service bit if we don't have a proof.Apr 22 2021, 06:38
deadalnix accepted this revision.Apr 22 2021, 11:16
This revision is now accepted and ready to land.Apr 22 2021, 11:16
This revision was landed with ongoing or failed builds.Apr 22 2021, 12:01
Closed by commit rABC35e52efad474: [avalanche] Move proof verification to processor (authored by Fabien). · Explain Why
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC35e52efad474: [avalanche] Move proof verification to processor.

Revision Contents
Changeset List

PathSize
src/
avalanche/
13 lines
96 lines
test/
8 lines
50 lines
test/
functional/
22 lines

Diff 28244

View Options

src/avalanche/processor.h

Loading...
View Options

src/avalanche/processor.cpp

Loading...
View Options

src/avalanche/test/processor_tests.cpp

Loading...
View Options

src/init.cpp

Loading...
View Options

test/functional/abc_rpc_avalancheproof.py

Loading...