Skip to content

Commit

Permalink
fix: broken uint256_t implicit copy (AztecProtocol#3625)
Browse files Browse the repository at this point in the history
Look at the diff, then consider this code
```
#include <iostream>
#include <concepts>

class Uint256_t {
public:
    // Conversion operator (explicit)
    template <std::integral T>
    explicit operator T() const {
        std::cout << "Uint256_t conversion operator called\n";
        return static_cast<T>(data[0]);
    }
    int data[4] = {0,0,0,0};
};

class Field {
public:
    Field() {}
    // Constructor that takes a Uint256_t
    Field(int x) {
        std::cout << "This is silly!!!!" << std::endl;
    }
    Field(const Uint256_t& input) {
        (void)input;
        std::cout << "Field constructor called\n";
        // Additional initialization code here
    }
};

class Toot {
    public:
    Toot(Field f) {
          (void)f;
    }
};

int main() {
    Uint256_t uint256_value;
    Field x = uint256_value;
    Toot t{uint256_value};

    return 0;
}
```

With std::integral concept on the cast:

```
Field constructor called
Field constructor called
```


but without! which was caught in practice passing a uint256 to
FieldSponge

```
Field constructor called
Uint256_t conversion operator called
This is silly!!!!
```

This truncated the field mysteriously! Open-ended implicit conversions
are dangerous

---------

Co-authored-by: ludamad <adam@aztecprotocol.com>
  • Loading branch information
ludamad and ludamad0 authored Dec 7, 2023
1 parent 2cede41 commit 1a6b44d
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#ifdef __i386__
#include "barretenberg/common/serialize.hpp"
#include <concepts>

namespace numeric {

Expand Down Expand Up @@ -37,7 +38,7 @@ class alignas(32) uint128_t {
constexpr ~uint128_t() = default;
explicit constexpr operator bool() const { return static_cast<bool>(data[0]); };

template <typename T> explicit constexpr operator T() const { return static_cast<T>(data[0]); };
template <std::integral T> explicit constexpr operator T() const { return static_cast<T>(data[0]); };

[[nodiscard]] constexpr bool get_bit(uint64_t bit_index) const;
[[nodiscard]] constexpr uint64_t get_msb() const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "../uint128/uint128.hpp"
#include "barretenberg/common/serialize.hpp"
#include "barretenberg/common/throw_or_abort.hpp"
#include <concepts>
#include <cstdint>
#include <iomanip>
#include <iostream>
Expand Down Expand Up @@ -91,7 +92,7 @@ class alignas(32) uint256_t {

explicit constexpr operator bool() const { return static_cast<bool>(data[0]); };

template <typename T> explicit constexpr operator T() const { return static_cast<T>(data[0]); };
template <std::integral T> explicit constexpr operator T() const { return static_cast<T>(data[0]); };

[[nodiscard]] constexpr bool get_bit(uint64_t bit_index) const;
[[nodiscard]] constexpr uint64_t get_msb() const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ template <typename Builder> class BigFieldBase {
mask = (uint256_t(1) << mask_size) - 1;
// Choose the bit range
// Return instruction
return { .id = instruction_opcode, .arguments.element = Element(temp & mask) };
return { .id = instruction_opcode, .arguments.element = Element(static_cast<uint64_t>(temp & mask)) };

break;
case OPCODE::RANDOMSEED:
Expand Down

0 comments on commit 1a6b44d

Please sign in to comment.