build: fix build with Boost 1.85 and remove instances of viewkey logging #9450

jeffro256 · 2024-08-23T17:18:44Z

Use std::is_standard_layout and std::is_trivially_copyable instead of std::is_pod for KV byte-wise serialization, which fixes compile issue for Boost UUIDs
Use std::has_unique_object_representations instead of alignof(T) == 1 for epee byte spans and epee hex functions
Removed reimplementation of std::hash for boost::uuids::uuid
Removed << operator overload for crypto::secret_key
Removed instances in code where private view key was dumped to the log in plaintext

vtnerd · 2024-08-23T17:55:52Z

This was a correct compile-time check, POD implied standard layout and trivial. Now you have to check for multiple things as you do here. One thing I noticed is that I didn't check for trivial copyable in epee::span, I'll look into upgrading that separately.

The epee::span check also does an alignof(...) == 1 in a crappy way to identify padding. I'm not sure if this can be added here without breaking compilation.

jeffro256 · 2024-08-23T18:53:02Z

POD implied standard layout and trivial

You're right, it's just that its probably too tight of property for what we need. namely, to do blob serialization, we don't care if the class is trivial as long as it is trivially copyable.

One thing I noticed is that I didn't check for trivial copyable in epee::span, I'll look into upgrading that separately.

We might want to change it here in this PR because Boost 1.85 also broke static assertions in other parts of the codebase: https://github.com/monero-project/monero/actions/runs/10529735156/job/29178145531?pr=9450.

jeffro256 · 2024-08-24T03:45:07Z

Okay so sorry for the spam, but it should be ready now

jeffro256 · 2024-08-24T04:29:05Z

Side note: we use a variable time algorithm for converting some of these secrets into hex which opens us up to cache timing attacks. Not a huge deal since we usually only hexify tx ephemeral privkeys (except in the wallet save code), and/or it can't be trivially triggered repeatedly. However, this is far from ideal.

selsta · 2024-08-25T20:11:21Z

can confrim this solves compilation issues for me, please squash

jeffro256 · 2024-08-26T17:43:37Z

Squashed and implemented @vtnerd's suggestions for epee byte span functions

0xFFFC0000 · 2024-08-26T17:57:19Z

CMakeLists.txt

@@ -1091,6 +1091,7 @@ endif()
 find_package(Boost 1.58 QUIET REQUIRED COMPONENTS ${BOOST_COMPONENTS})
 add_definitions(-DBOOST_ASIO_ENABLE_SEQUENTIAL_STRAND_ALLOCATION)
 add_definitions(-DBOOST_NO_AUTO_PTR)
+add_definitions(-DBOOST_UUID_DISABLE_ALIGNMENT) # This restores UUID's std::has_unique_object_representations property


Can we make this target dependent instead of universal definition?

Roughly:

target_compile_definitions(boost::uuid PUBLIC -DBOOST_UUID_DISABLE_ALIGNMENT)

Or something like this:

set_property(TARGET boost::uuid APPEND PROPERTY INTERFACE_COMPILE_DEFINITIONS -DBOOST_UUID_DISABLE_ALIGNMENT)

The main objective is preventing polluting the global flag namespace.

We need this flag for all translation units which include boost/uuid/uuid.hpp

selsta

Please also open against release-v0.18

jeffro256 · 2024-08-27T05:49:16Z

This PR requires C++17, which is not yet supported on the release branch

…ing [RELEASE] 1. Use `std::is_standard_layout` and `std::is_trivially_copyable` instead of `std::is_pod` for KV byte-wise serialization, which fixes compile issue for Boost UUIDs 2. Removed reimplementation of `std::hash` for `boost::uuids::uuid` 3. Removed `<<` operator overload for `crypto::secret_key` 4. Removed instances in code where private view key was dumped to the log in plaintext Release version of monero-project#9450

vtnerd · 2024-08-29T13:50:24Z

src/cryptonote_core/cryptonote_tx_utils.cpp

@@ -484,7 +484,7 @@ namespace cryptonote
          crypto::generate_ring_signature(tx_prefix_hash, boost::get<txin_to_key>(tx.vin[i]).k_image, keys_ptrs, in_contexts[i].in_ephemeral.sec, src_entr.real_output, sigs.data());
        ss_ring_s << "signatures:" << ENDL;
        std::for_each(sigs.begin(), sigs.end(), [&](const crypto::signature& s){ss_ring_s << s << ENDL;});
-        ss_ring_s << "prefix_hash:" << tx_prefix_hash << ENDL << "in_ephemeral_key: " << in_contexts[i].in_ephemeral.sec << ENDL << "real_output: " << src_entr.real_output << ENDL;
+        ss_ring_s << "prefix_hash:" << tx_prefix_hash << ENDL << "in_ephemeral_key: " << rct::sk2rct(in_contexts[i].in_ephemeral.sec) << ENDL << "real_output: " << src_entr.real_output << ENDL;


There are unwrap functions to expose the inner type without copy - unwrap(unwrap(txkey.sec)). This occurs several times throughout this PR.

The function rct::sk2rct also doesn't copy, but unwrap is a definitely a lot cleaner

The function does a return by value - so it is definitely triggering the copy constructor in this situation.

Nevermind I missed the & by the function name. Yikes! This violates the aliasing rule, too bad I missed this.

Okay newest force push drops usage of rct::sk2rct to convert secret keys to rct::key and uses unwrap or a wrapper called crypto::secret_key_explicit_print_ref. I can look into making a PR to fix the strict aliasing later

woodser · 2024-08-30T18:40:15Z

Builds for me as well (on the release branch). Thanks.

woodser · 2024-09-06T12:31:24Z

Is this generally stable enough to be used / trusted with funds?

I'm not able to build on macOS otherwise, so I'm stuck without it.

From vtnerd's last review, it looks like we're awaiting a couple more changes.

jeffro256 · 2024-09-06T16:22:20Z

It's probably safe for funds, but I'm going to push a commit very soon that makes it better (i.e. still copyless) in the scenario where we fix the strict aliasing violations with the rct::x2rct and rct::rct2x functions.

1. Use `std::is_standard_layout` and `std::is_trivially_copyable` instead of `std::is_pod` for KV byte-wise serialization, which fixes compile issue for Boost UUIDs 2. Use `std::has_unique_object_representations` instead of `alignof(T) == 1` for epee byte spans and epee hex functions 3. Removed reimplementation of `std::hash` for `boost::uuids::uuid 4. Removed `<<` operator overload for `crypto::secret_key` 5. Removed instances in code where private view key was dumped to the log in plaintext

jeffro256 · 2024-09-06T16:52:26Z

Windows build failure is unrelated to this PR

…ing [RELEASE] 1. Use std::is_standard_layout and std::is_trivially_copyable instead of std::is_pod for KV byte-wise serialization, which fixes compile issue for Boost UUIDs 2. Removed reimplementation of std::hash for boost::uuids::uuid 3. Removed << operator overload for crypto::secret_key 4. Removed instances in code where private view key was dumped to the log in plaintext Release version of monero-project#9450, containing C++14 modified assertions

jeffro256 force-pushed the fix_kvser_boost_158 branch from 5259d00 to e80e71f Compare August 23, 2024 17:20

jeffro256 force-pushed the fix_kvser_boost_158 branch from e80e71f to f4ff8b1 Compare August 23, 2024 20:43

jeffro256 changed the title ~~epee: fix serialization compile errors with Boost 1.85~~ build: fix build with Boost 1.85 and remove instances of viewkey logging Aug 23, 2024

jeffro256 force-pushed the fix_kvser_boost_158 branch 2 times, most recently from 64d5655 to 75f1810 Compare August 23, 2024 23:06

jeffro256 force-pushed the fix_kvser_boost_158 branch from 8eec319 to 4d6a3c0 Compare August 26, 2024 17:42

0xFFFC0000 reviewed Aug 26, 2024

View reviewed changes

0xFFFC0000 added enhancement pending review labels Aug 26, 2024

selsta approved these changes Aug 26, 2024

View reviewed changes

jeffro256 mentioned this pull request Aug 28, 2024

build: fix build with Boost 1.85 and remove instances of viewkey logging [RELEASE] #9462

Open

0xFFFC0000 approved these changes Aug 29, 2024

View reviewed changes

vtnerd reviewed Aug 29, 2024

View reviewed changes

selsta mentioned this pull request Aug 30, 2024

error: static assertion failed due to requirement 'std::is_pod<boost::uuids::uuid>::value': t_type must be a POD type on mac mini #9465

Closed

woodser mentioned this pull request Aug 30, 2024

building static libraries with shared library wrapper woodser/monero-java#105

Open

jeffro256 force-pushed the fix_kvser_boost_158 branch from 4d6a3c0 to ed955bf Compare September 6, 2024 16:47

selsta removed the pending review label Sep 10, 2024

selsta approved these changes Sep 10, 2024

View reviewed changes

0xFFFC0000 mentioned this pull request Sep 15, 2024

boost update 1.86 breaks the build (...again) #9485

Closed

selsta mentioned this pull request Sep 15, 2024

debug build: epee-related build error on macOS M1 #9486

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: fix build with Boost 1.85 and remove instances of viewkey logging #9450

build: fix build with Boost 1.85 and remove instances of viewkey logging #9450

jeffro256 commented Aug 23, 2024 •

edited

Loading

vtnerd commented Aug 23, 2024

jeffro256 commented Aug 23, 2024

jeffro256 commented Aug 24, 2024

jeffro256 commented Aug 24, 2024 •

edited

Loading

selsta commented Aug 25, 2024

jeffro256 commented Aug 26, 2024

0xFFFC0000 Aug 26, 2024

jeffro256 Aug 26, 2024

selsta left a comment

jeffro256 commented Aug 27, 2024

vtnerd Aug 29, 2024

jeffro256 Aug 29, 2024

vtnerd Aug 31, 2024

vtnerd Aug 31, 2024

jeffro256 Sep 6, 2024

woodser commented Aug 30, 2024

woodser commented Sep 6, 2024

jeffro256 commented Sep 6, 2024

jeffro256 commented Sep 6, 2024

build: fix build with Boost 1.85 and remove instances of viewkey logging #9450

Are you sure you want to change the base?

build: fix build with Boost 1.85 and remove instances of viewkey logging #9450

Conversation

jeffro256 commented Aug 23, 2024 • edited Loading

vtnerd commented Aug 23, 2024

jeffro256 commented Aug 23, 2024

jeffro256 commented Aug 24, 2024

jeffro256 commented Aug 24, 2024 • edited Loading

selsta commented Aug 25, 2024

jeffro256 commented Aug 26, 2024

0xFFFC0000 Aug 26, 2024

Choose a reason for hiding this comment

jeffro256 Aug 26, 2024

Choose a reason for hiding this comment

selsta left a comment

Choose a reason for hiding this comment

jeffro256 commented Aug 27, 2024

vtnerd Aug 29, 2024

Choose a reason for hiding this comment

jeffro256 Aug 29, 2024

Choose a reason for hiding this comment

vtnerd Aug 31, 2024

Choose a reason for hiding this comment

vtnerd Aug 31, 2024

Choose a reason for hiding this comment

jeffro256 Sep 6, 2024

Choose a reason for hiding this comment

woodser commented Aug 30, 2024

woodser commented Sep 6, 2024

jeffro256 commented Sep 6, 2024

jeffro256 commented Sep 6, 2024

jeffro256 commented Aug 23, 2024 •

edited

Loading

jeffro256 commented Aug 24, 2024 •

edited

Loading