Page MenuHomePhabricator

[avalanche] Add an option to import a delegation
ClosedPublic

Authored by Fabien on Apr 2 2021, 08:45.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABCbc20bfeeb2f7: [avalanche] Add an option to import a delegation
Summary

The "-avadelegation" option let the user supply an already built
delegation to the node, so the proof can be used without passing the
master private key around.

Depends on D9445.

Ref T1635.

Test Plan
ninja check-functional

Diff Detail

Event Timeline

Fabien requested review of this revision.Apr 2 2021, 08:45
deadalnix requested changes to this revision.Apr 3 2021, 19:57
deadalnix added a subscriber: deadalnix.

What problem removing the distinction between master key and session key solves? It seems to me like it's making security of the whole thing worse for no good reason.

src/avalanche/processor.cpp
453

Why are you making a copy of the whole delegation?

src/init.cpp
1237

Why? This behavior change doesn't seem to serve any other purpose than adding a special case.

Presumably, the delegation delegates to a key. How is the node supposed to have the corresponding private key?

2480

It doesn't seem to make sense to me to accept an invalid delegation, then require init.cpp to know that the delegation might be invalid and check it. Why does init.cpp need to know about the delegation at all, past the fact that there is a command line argument about it?

This revision now requires changes to proceed.Apr 3 2021, 19:57

Revert to original master key/session key intent, rebase on top of D9422 to remove verification from init.cpp

deadalnix requested changes to this revision.Apr 20 2021, 00:06
deadalnix added inline comments.
src/avalanche/processor.cpp
227 ↗(On Diff #28194)

You might have built an invalid delegation here, no? What if the provided delegation doesn't match the master key provided?

Most likely, you should check correctness at the end, not in the middle of things.

src/init.cpp
1233 ↗(On Diff #28194)

To the master key. the session key by default is randomly generated, unless specified otherwise.

test/functional/abc_rpc_avalancheproof.py
225 ↗(On Diff #28194)

You get it right in the test, so just set descriptions accordingly.

This revision now requires changes to proceed.Apr 20 2021, 00:06

Fix missed session key vs master key confusion, reorder the checks to make sure no wrong delegation can be generated

deadalnix requested changes to this revision.Apr 23 2021, 16:07
deadalnix added inline comments.
src/avalanche/processor.cpp
259–262 ↗(On Diff #28252)

remove

298 ↗(On Diff #28252)

This seems to be unrelated with what the diff is doing and needs to be in its own diff. The whole design also seems fairly redundant, cutting in bit will make it more obvious where the fat is.

This revision now requires changes to proceed.Apr 23 2021, 16:07

Remove debug prints, only keep the new option part of the diff. The additional tests will be extracted in a follow up.

deadalnix requested changes to this revision.Apr 26 2021, 20:56
deadalnix added inline comments.
src/avalanche/processor.cpp
261 ↗(On Diff #28267)

In what way? is this actually checking if the delegation is valid? That seems like out of scope for that function. Most likely, the error message is wrong, but the fact I can't tell for sure.

This revision now requires changes to proceed.Apr 26 2021, 20:56

Update the error message

deadalnix requested changes to this revision.Apr 29 2021, 14:20
deadalnix added inline comments.
src/avalanche/processor.cpp
258 ↗(On Diff #28306)

What happens when this throws because the delegation is not hex or does not represent a delegation at all?

261 ↗(On Diff #28306)

This is not a new problem here, but none of these errors match the existing style passed down to InitError .

261 ↗(On Diff #28306)

What does it mean that the delegation failed to import? Why? What do I need to do if the node spit that message back to me? I can't even tell from reading this part of the source code, how any users is supposed to be able to do anything with this?

This revision now requires changes to proceed.Apr 29 2021, 14:20
Fabien planned changes to this revision.Jun 4 2021, 20:01

Rebase. The updated version for this diff and the compagnon D9445 should address all the feedback

src/avalanche/processor.cpp
287 ↗(On Diff #28872)

Why does the delegation builder goes through a unique_ptr? It doesn't looks like this is necessary.

src/avalanche/processor.cpp
287 ↗(On Diff #28872)

I need to use a different constructor depending on whether a delegation is provided or not, then I have some common path that uses this delegation builder. The unique pointer is used to declare the DelegationBuilder outside of the branches scopes.

deadalnix added inline comments.
src/avalanche/processor.cpp
287 ↗(On Diff #28872)

That sounds like a terrible reason to do it this way.

This revision is now accepted and ready to land.Jun 15 2021, 12:46