Skip to content

Conversation

NickeZ
Copy link
Collaborator

@NickeZ NickeZ commented Mar 13, 2020

libwally was compiling with -03. no bueno.

old:
   text    data     bss     dec     hex filename
 452980   22924  208352  684256   a70e0 firmware.elf

new:
   text    data     bss     dec     hex filename
 443532   22932  208352  674816   a4c00 firmware.elf

@NickeZ NickeZ requested a review from benma March 13, 2020 14:02
@benma
Copy link
Collaborator

benma commented Mar 14, 2020

libwally is the lib which performs the slow bip39 unlock, and also ships secp256k1. did you check if unlock does not take longer, and that signing a tx works?

${CMAKE_CURRENT_SOURCE_DIR}/libwally-core/configure
${CONFIGURE_FLAGS}
${LIBWALLY_CONFIGURE_FLAGS}
COMMAND find ${CMAKE_CURRENT_BINARY_DIR}/libwally-core -name Makefile | xargs sed -i "s/-O3/-Os/"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very not nice :D Is there really no way to configure it or pass it as an env var or sth?

otherwise you can also make a new branch here: https://github.com/shiftdevices/libwally-core

@NickeZ
Copy link
Collaborator Author

NickeZ commented Mar 15, 2020

  • I'll have to check that libwally operations don't take significantly longer.
  • It isn't possible to change the optimization flags for libwally, they have only 2 options, debug -Og and non-debug -O3.
  • Using -O3 in general is absolute madness. They should be using -O2. -O3 enables new and exciting optimizations which hasn't been battle tested. It is well known to sometimes pessimize code.

I'll patch our fork of libwally instead of monkeypatching like this though..

@benma
Copy link
Collaborator

benma commented Mar 15, 2020

In this case I suggest to fix it upstream, and not do this change for now. Seems a bit premature, we are not strapped for space yet.

@NickeZ
Copy link
Collaborator Author

NickeZ commented Apr 2, 2020

Solved upstream: ElementsProject/libwally-core#182

They agreed that -O3 is wrong and overriding the user is wrong. So now it is possible to set optimization level with CFLAGS.

@NickeZ NickeZ closed this Apr 2, 2020
@x1ddos
Copy link
Contributor

x1ddos commented Apr 2, 2020

Solved upstream: ElementsProject/libwally-core#182

Still need to pull it in and update commit hash in https://github.com/digitalbitbox/bitbox02-firmware/tree/master/external, right.

Also, would be nice if url entries in https://github.com/digitalbitbox/bitbox02-firmware/blob/26a8b821aad9814007b9ff5dacf043f8883ced70/.gitmodules were updated to point to github/digitalbitbox/repo instead of shiftdevices.

@NickeZ
Copy link
Collaborator Author

NickeZ commented Apr 3, 2020

Sure, but both those things are unrelated to this PR. Make comments / create issues instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants