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.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- Restricted Maniphest Task
- Commits
- rABCbc20bfeeb2f7: [avalanche] Add an option to import a delegation
ninja check-functional
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- import_delegation
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 15440 Build 30801: Build Diff build-debug · build-diff · lint-circular-dependencies · build-clang-tidy · build-without-wallet · build-clang Build 30800: arc lint + arc unit
Event Timeline
Comment Actions
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? | |
Comment Actions
Revert to original master key/session key intent, rebase on top of D9422 to remove verification from init.cpp
| 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. |
Comment Actions
Fix missed session key vs master key confusion, reorder the checks to make sure no wrong delegation can be generated
Comment Actions
Remove debug prints, only keep the new option part of the diff. The additional tests will be extracted in a follow up.
| 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. |
| 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? |
Comment Actions
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. |
| src/avalanche/processor.cpp | ||
|---|---|---|
| 287 ↗ | (On Diff #28872) | That sounds like a terrible reason to do it this way. |