cardano: Allow vote delegation #1299

RostarMarek · 2024-09-04T11:18:29Z

Allow cardano based on conway.cddl to sign messages containing Vote Delegation certificate. This is needed for withdrawing rewards.

I will add tests for firmware soon, just wanted to begin the review process as soon as possible. It has been tested through simulator and works as expected.

RostarMarek · 2024-09-04T11:20:07Z

Tried testing it through devdevice, but even in docker environment when running make firmware and make bootloader there was an error that was strange

dockeruser@aea6becef83c:/Users/marekrostar/git/forks/bitbox02-firmware$ make firmware
mkdir -p build
cd build && cmake -DCMAKE_TOOLCHAIN_FILE=arm.cmake ..
-- size: /opt/homebrew/bin/arm-none-eabi-size arm-none-eabi-size
CMake Error at CMakeLists.txt:116 (message):
  get_version firmware failed


-- Configuring incomplete, errors occurred!
See also "/Users/marekrostar/git/forks/bitbox02-firmware/build/CMakeFiles/CMakeOutput.log".
See also "/Users/marekrostar/git/forks/bitbox02-firmware/build/CMakeFiles/CMakeError.log".
make: *** [Makefile:26: build/Makefile] Error 1
dockeruser@aea6becef83c:/Users/marekrostar/git/forks/bitbox02-firmware$ make bootloader
mkdir -p build
cd build && cmake -DCMAKE_TOOLCHAIN_FILE=arm.cmake ..
-- size: /opt/homebrew/bin/arm-none-eabi-size arm-none-eabi-size
CMake Error at CMakeLists.txt:116 (message):
  get_version firmware failed


-- Configuring incomplete, errors occurred!
See also "/Users/marekrostar/git/forks/bitbox02-firmware/build/CMakeFiles/CMakeOutput.log".
See also "/Users/marekrostar/git/forks/bitbox02-firmware/build/CMakeFiles/CMakeError.log".
make: *** [Makefile:26: build/Makefile] Error 1
dockeruser@aea6becef83c:/Users/marekrostar/git/forks/bitbox02-firmware$ make bootloader-devdevice
make: *** No rule to make target 'bootloader-devdevice'.  Stop.
dockeruser@aea6becef83c:/Users/marekrostar/git/forks/bitbox02-firmware$ make firmware
mkdir -p build
cd build && cmake -DCMAKE_TOOLCHAIN_FILE=arm.cmake ..
-- size: /opt/homebrew/bin/arm-none-eabi-size arm-none-eabi-size
CMake Error at CMakeLists.txt:116 (message):
  get_version firmware failed


-- Configuring incomplete, errors occurred!
See also "/Users/marekrostar/git/forks/bitbox02-firmware/build/CMakeFiles/CMakeOutput.log".
See also "/Users/marekrostar/git/forks/bitbox02-firmware/build/CMakeFiles/CMakeError.log".
make: *** [Makefile:26: build/Makefile] Error 1
dockeruser@aea6becef83c:/Users/marekrostar/git/forks/bitbox02-firmware$ ./scripts/get_version 
py-bitbox02-6.3.0+200.dirty
dockeruser@aea6becef83c:/Users/marekrostar/git/forks/bitbox02-firmware$ ./scripts/get_version firmware
v9.19.0+131.dirty
dockeruser@aea6becef83c:/Users/marekrostar/git/forks/bitbox02-firmware$ ./scripts/get_version firmware --check-semver --check-gpg
v9.19.0+131.dirty

looking into the error logs this error was reported

/usr/local/bin/../lib/gcc/arm-none-eabi/8.2.1/../../../../arm-none-eabi/bin/ld: /usr/local/bin/../lib/gcc/arm-none-eabi/8.2.1/../../../../arm-none-eabi/lib/libc.a(lib_a-exit.o): in function `exit':
exit.c:(.text.exit+0x2c): undefined reference to `_exit'
collect2: error: ld returned 1 exit status

which seems like the cmake have and the gcc specifically have some sort of internal error.

NickeZ · 2024-09-04T11:32:45Z

Did you run this command? git config --global --add safe.directory <the path to project>? I seem to have to run it every time I run a new container.

RostarMarek · 2024-09-04T11:36:26Z

Did you run this command? git config --global --add safe.directory <the path to project>? I seem to have to run it every time I run a new container.

Not initially, but it was needed to build the simulator so eventually yes. The command make simulator -j works.

benma · 2024-09-04T11:36:27Z

Did you run this command? git config --global --add safe.directory <the path to project>? I seem to have to run it every time I run a new container.

Interesting, I guess I am not running into this as I use podman 🤔

We apply this fix here btw:

bitbox02-firmware/releases/build.sh

Line 51 in b399d44

    
           docker run -it --rm --volume `pwd`:/bb02 bitbox02-firmware bash -c "git config --global --add safe.directory /bb02 && cd /bb02 && $2"

Maybe it could be added in dockerenv.sh too so it automatically runs there too.

benma

Thanks!

I left some early comments, but the direction seems good!

benma · 2024-09-04T11:39:56Z

messages/cardano.proto

+  KeyHash = 0;
+  ScriptHash = 1;
+  AlwaysAbstain = 2;
+  AlwaysNoConfidence = 3;


Unfortunately protobuf enum fields live in the global namespace, so all fields need a namespace prefix :( Please add CARDANO_DREP_TYPE_. In Rust, the protobuf generator will strip the prefix again I think, so it's not too bad.

If this message will never be used outside of the VoteDelegation message, you can also define it within VoteDelegation. In that case, you don't need the prefix, as the enum will be scoped.

Afaik the naming convention for enum fields is all uppercase too, so KEY_HASH, etc.

I do not think that in forseeable future with the current business case I was told of this will be used elsewhere, so I moved it into VoteDelegation. I put it on top with the casing based on other protobuf files and the naming in CardanoNetwork to keep consistency, but this should be nicer.

Thanks. Indeed, CardanoNetwork uses a different style unfortunately. I checked, most other enums in the project are uppercase.

benma · 2024-09-04T11:41:20Z

messages/cardano.proto

  message Certificate {
    message StakeDelegation {
-      repeated uint32 keypath = 1;
+      repeated uint32 stake_keypath = 1;


Seems like an unrelated change that breaks downstream libraries. Is this necessary?

It was suggested during internal review to make naming consistent and in vote delegation it makes sense. I am not completely partial to it so I fine with keeping the naming keypath in StakeDelegation certificate

This is also what seemed to break CI:

--> bitbox02-rust/src/hww/api/cardano/sign_transaction.rs:526:29 | 526 | ... keypath: vec![1852 + HARDENED, 1815 + HARDENED, HARDENED, 2, 0], | ^^^^^^^ `StakeDelegation` does not have this field |

I'd leave it as it was, as otherwise:

renaming it here means that downstream libraries need to change and get a major version bump, and the Adalite integration with the bitbox-api NPM package then also needs to update the name.

this update would seem to be inconsistent with the stake_registration and stake_deregistration fields, where the name is also still keypath.

Could it make sense to simply name the VoteDelegation keypath field keypath instead for consistency?

The TypeScript types right now are derived from the .proto field names like this:

type CardanoCertificate = | { stakeRegistration: { keypath: Keypath } } | { stakeDeregistration: { keypath: Keypath } } | { stakeDelegation: { keypath: Keypath poolKeyhash: Uint8Array } };

I think it can make sense, but I will add a comment above it so at least in code there is a comment that it is supposed to be stake_keypath

benma · 2024-09-04T11:57:53Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/cbor.rs

+                    else if matches!(r#type,2){
+                        encoder.array(1)?.u8(2)?;
+                    }
+                    else if matches!(r#type,3){


please apply rustfmt on save to format the Rust code.

benma · 2024-09-04T12:00:49Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/cbor.rs

+                }) => {
+                    encoder.array(3)?.u8(9)?;
+                    encode_stake_credential(&mut encoder, stake_keypath)?;
+                    if matches!(r#type,0){


you can decode this to have a nicer pattern matching:

let drep_type = CardanoDRepType::try_from(r#type)?; match drep_type { CardanoDRepType::KeyHash => { ... } // etc }

benma · 2024-09-04T12:13:50Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/cbor.rs

+                        encoder.array(2)?.u8(0)?;
+                        match drep_credhash {
+                            Some(hash) => {
+                                encoder.bytes(hash)?;


should also check the expected length of the hash.

instead of making it optional ..., you could remove that keyword and check the length only. clients that omit it default to the empty bytestring.

Added checking of length and I think using optional makes more sense here and makes it a bit more readable, maybe just because I dont like to rely on defaults from languages/libraries.

I'm thinking of adding a check in the cases where there should not be a drep_credhash sent to check it is none or throw an error. Functionality wise it does not change anything(even if it was sent it will not be encoded), but it might be nice to keep the requests clean. What do you think?

Sounds good to me 👍

benma · 2024-09-06T06:53:38Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/cbor.rs

+                                Some(hash) => {
+                                    if hash.len() != 28 {
+                                        return Err(Error::InvalidInput);
+                                    }


can also shorten this as Some(hash) if hash.len() == 28 => { ... }

RostarMarek · 2024-09-06T11:35:47Z

Also just to let it be known here this PR adds support for the whole certificate, Ada lite currently supports only AlwaysAbstain but I think it is more valid to add support for all possible options instead of allowing just this option in firmware.

benma · 2024-09-17T10:26:37Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/certificates.rs

+            }) => {
+                validate_address_shelley_stake(keypath, Some(bip44_account))?;
+                signing_keypaths.push(keypath);
+                confirm::confirm(&confirm::Params {


Don't we also have to confirm the drep type and drep credhash?

Added a second confirm screen to do this, I didn't want to merge into one because it made it a bit unreadable in my opinion.

benma

Please rebase, squash the commits and also add a line to CHANGELOG.md. Thanks!

benma

Please also rebase again, a change was merged that modifies some protobuf files too, it will make it easire to copy them to bitbox-api-rs.

benma · 2024-09-18T12:34:40Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/certificates.rs

+                    Some(hash) => { confirm::confirm(&confirm::Params {
+                        title: params.name,
+                        body: &format!(
+                            "to type #{} and drep #{}?",


why the hashtag #?

I just now realized it was there to signify number in the message so I removed them.

benma · 2024-09-18T12:36:38Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/certificates.rs

+                confirm::confirm(&confirm::Params {
+                        title: params.name,
+                        body: &format!(
+                            "Delegate voting for account #{}?",


I think UX would be a bit better if it was just a single line - it is already scrollable. Then the user does not have to worry about which type/drep it is, they can't see ahead.

If you think two screens are better anyway, it's better to not start it with "to ...", but maybe simply "Type: .... DRep: ...".

I am split as I think 2 screens are a little bit more readable, but also a bit more confusing as I am not sure how many users will actually know what which type of delegation means so it might just unnecessarily confuse them. I made it 1 screen in the end.

benma

final nit before merge 😇

benma · 2024-09-18T13:03:24Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/certificates.rs

+                    Some(hash) => { confirm::confirm(&confirm::Params {
+                        title: params.name,
+                        body: &format!(
+                            "Delegate voting for account #{} to type {} and drep {}??",


typo: double ?? 😄

Yes sorry, fix pushed

Allow cardano based on conway.cddl to sign messages containing Vote Delegation certificate. This is needed for withdrawing rewards. Signed-off-by: RostarMarek <rostarmarek@gmail.com> cardano: Add Test and minor improvements Adds unit test for vote delegation and adresses minor issues mentioned in PR. Signed-off-by: RostarMarek <rostarmarek@gmail.com> cardano: Adress review issues Addresses review issues found during the review process Signed-off-by: RostarMarek <rostarmarek@gmail.com> cardano: Add confirmation screen Adds confirmation screen for drep vote delegation dislpaying type and if present drep credential hash. Signed-off-by: RostarMarek <rostarmarek@gmail.com>

benma · 2024-09-18T13:11:29Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/cbor.rs

+                                Some(_hash) => return Err(Error::InvalidInput),
+                                None => return Err(Error::InvalidInput),


could collapse these into one _ => return Err ... arm. Same above

benma · 2024-09-18T13:17:15Z

src/rust/bitbox02-rust/src/hww/api/cardano/sign_transaction/cbor.rs

+                    let drep_type = certificate::vote_delegation::CardanoDRepType::try_from(*r#type)?;
+                    match drep_type {
+                        certificate::vote_delegation::CardanoDRepType::KeyHash => {
+                            encoder.array(2)?.u8(0)?;


I have no good of way of verifying this is correct, so we'll see in an end-to-end test, but

https://github.com/IntersectMBO/cardano-ledger/blob/cardano-ledger-conway-1.12.0.0/eras/conway/impl/cddl-files/conway.cddl#L273

vote_deleg_cert = (9, stake_credential, drep)

I guess the encoder.array(3?).u8(9)? above starts the array and adds the first element. Shouldn't the stake_credential and drep now be added directly as two elements? Here it seems like you are adding them as an array of 2 elements.

Have you verified somehow that the cbor output here is correct? Could very well be that I am just mis-reading this, it's a bit hairy 😅

Nevermind, I just realized the stake_credential part is itself an array of 2!

benma

LGTM, thanks! Nice job!

RostarMarek force-pushed the rostarmarek/cardano-vote-deleg branch from 9581584 to 85113f5 Compare September 4, 2024 11:22

benma reviewed Sep 4, 2024

View reviewed changes

RostarMarek force-pushed the rostarmarek/cardano-vote-deleg branch from d9627ba to 249bfaa Compare September 5, 2024 14:35

benma reviewed Sep 6, 2024

View reviewed changes

benma reviewed Sep 17, 2024

View reviewed changes

RostarMarek force-pushed the rostarmarek/cardano-vote-deleg branch 2 times, most recently from 9ca59e2 to 47fc205 Compare September 18, 2024 09:41

benma requested changes Sep 18, 2024

View reviewed changes

RostarMarek force-pushed the rostarmarek/cardano-vote-deleg branch 2 times, most recently from 727722f to 1456c75 Compare September 18, 2024 12:46

benma requested changes Sep 18, 2024

View reviewed changes

RostarMarek force-pushed the rostarmarek/cardano-vote-deleg branch from 1456c75 to d0e3716 Compare September 18, 2024 13:06

benma reviewed Sep 18, 2024

View reviewed changes

benma approved these changes Sep 18, 2024

View reviewed changes

benma merged commit d966071 into BitBoxSwiss:master Sep 18, 2024
3 checks passed

RostarMarek mentioned this pull request Sep 27, 2024

Cardano: Add vote delegation to node api BitBoxSwiss/bitbox-api-rs#89

Open

		Some(_hash) => return Err(Error::InvalidInput),
		None => return Err(Error::InvalidInput),

cardano: Allow vote delegation #1299

cardano: Allow vote delegation #1299

Conversation

RostarMarek commented Sep 4, 2024 • edited Loading

RostarMarek commented Sep 4, 2024 • edited Loading

NickeZ commented Sep 4, 2024

RostarMarek commented Sep 4, 2024

benma commented Sep 4, 2024

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RostarMarek commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma left a comment • edited Loading

Choose a reason for hiding this comment

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma left a comment

Choose a reason for hiding this comment

RostarMarek commented Sep 4, 2024 •

edited

Loading

RostarMarek commented Sep 4, 2024 •

edited

Loading

benma left a comment •

edited

Loading