Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve #88 #109

Merged
merged 3 commits into from
Mar 19, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions libraries/eosiolib/capi/eosio/action.h
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,16 @@ uint64_t publication_time( void );
__attribute__((eosio_wasm_import))
capi_name current_receiver( void );

/**
* Get the hash of the code currently published on the given account
* @param account Name of the account to hash the code of
* @param struct_version Version specifying the desired format of the returned struct
* @param result_buffer Buffer wherein the result should be written
* @param buffer_size Size in bytes of result_buffer
*/
__attribute__((eosio_wasm_import))
uint32_t get_code_hash( uint64_t account, uint32_t struct_version, char* result_buffer, size_t buffer_size );

/**
* Set the action return value which will be included in the action_receipt
* @brief Set the action return value
Expand Down
39 changes: 39 additions & 0 deletions libraries/eosiolib/contracts/eosio/action.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "../../core/eosio/serialize.hpp"
#include "../../core/eosio/datastream.hpp"
#include "../../core/eosio/name.hpp"
#include "../../core/eosio/fixed_bytes.hpp"
#include "../../core/eosio/ignore.hpp"
#include "../../core/eosio/time.hpp"

Expand Down Expand Up @@ -52,9 +53,23 @@ namespace eosio {

__attribute__((eosio_wasm_import))
uint64_t current_receiver();

__attribute__((eosio_wasm_import))
uint32_t get_code_hash( uint64_t account, uint32_t struct_version, char* result_buffer, size_t buffer_size );
}
};

struct code_hash_result {
unsigned_int struct_version;
uint64_t code_sequence;
checksum256 code_hash;
uint8_t vm_type;
uint8_t vm_version;

CDT_REFLECT(struct_version, code_sequence, code_hash, vm_type, vm_version);
EOSLIB_SERIALIZE(code_hash_result, (struct_version)(code_sequence)(code_hash)(vm_type)(vm_version));
};

/**
* @defgroup action Action
* @ingroup contracts
Expand Down Expand Up @@ -150,6 +165,30 @@ namespace eosio {
return name{internal_use_do_not_use::current_receiver()};
}

/**
* Get the hash of the code currently published on the given account
* @param account Name of the account to hash the code of
* @param full_result Optional: If a full result struct is desired, a pointer to the struct to populate
* @return The SHA256 hash of the specified account's code
*/
inline checksum256 get_code_hash( name account, code_hash_result* full_result = nullptr ) {
if (full_result == nullptr)
full_result = (code_hash_result*)alloca(sizeof(code_hash_result));

// Packed size is dynamic, so we don't know what it'll be. Try to have plenty of space.
auto struct_buffer_size = sizeof(code_hash_result)*2;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure which one is more efficient but we have a common pattern to execute intrinsic where first we execute it with zero size first, it returns with return structure size and then we execute it again. example is get_action:

   inline action get_action( uint32_t type, uint32_t index ) {
      constexpr size_t max_stack_buffer_size = 512;
      int s = internal_use_do_not_use::get_action( type, index, nullptr, 0 );
      eosio::check( s > 0, "get_action size failed" );
      size_t size = static_cast<size_t>(s);
      char* buffer = (char*)( max_stack_buffer_size < size ? malloc(size) : alloca(size) );
      auto size2 = internal_use_do_not_use::get_action( type, index, buffer, size );
      eosio::check( size == static_cast<size_t>(size2), "get_action failed" );
      return eosio::unpack<eosio::action>( buffer, size );
   }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, well I'm not sure how the overheads stack up in the WASM VM, but calling an intrinsic is roughly equivalent in principle to a syscall to kernel space from userland, and those are notoriously slow. alloca, on the other hand, is merely ticking a register, which is pretty much the fastest instruction imaginable.

There's no substitute for benchmarking, but I would expect making an extra call out of the VM to be at least a couple of orders of magnitude slower than alloca. I think it would only make sense to make the second call if there was a possibility that the struct would be unpredictably large (especially, structs containing string or vector, etc), but even in this case, just allocating a very large heap buffer is probably much faster, just more wasteful of space. That's a hard decision for a library to make, since different clients have different constraints, but in this case the buffer's size has a tight upper bound so we don't need to concern ourselves. =)

Copy link
Contributor Author

@nathanielhourt nathanielhourt Mar 2, 2023

Choose a reason for hiding this comment

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

Is there a way to get microseconds or CPU ticks within a contract? I'd like to run some benchmarks, including the cost of calling an intrinsic, as I'm pretty certain it falls in the category of obnoxiously slow, and it might be worth my effort to go through and replace all of these "when you call the intrinsic once, call it twice" instances with something way faster. CPU time is scarce on the blockchain, so the stdlib should make an effort to conserve it for the contract.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to get microseconds or CPU ticks within a contract?

Seems like that would be a good addition to: eosnetworkfoundation/product#134

Copy link
Contributor

Choose a reason for hiding this comment

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

@nathanielhourt I agree with your point but the easiest solution looks to me just to make it inline with other stuff. Anyway there are not a lot. If you can spend extra and tell the difference, we can go that way and redo this (probably as part of other PR).
@larryk85 any thought on that?

Copy link
Member

Choose a reason for hiding this comment

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

Since the packed size will always be the same unless we somehow manage to make it to 128 versions of the struct, which seems incredibly unlikely since each bump would be a consensus change, it does seem rather wasteful to make multiple calls. It seems unlikely to even need to *2 the size here.

I might suggest checking the return value of internal_use_do_not_use::get_code_hash(), maybe even in lieu of the version check (the possibility that the host function could in the future return a version different than the one asked for would seem to be devastating to existing contracts -- why would we ever do that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hahaha yeah, the version check is abysmally pessimistic, but I like to make such checks where cheap. Bugs love to hide at API boundaries and deucedly so at versioned ones! =)

The *2 size is definitely overkill, and I would be amenable to reducing it. I also agree that checking the return value is prudent. Eventually, a general template could probably be improvised to do an optimistic fast path (alloca and single call) with a slow path fallback (malloc and second call) if the first call didn't allocate adequate space, and even give motivated clients the option to pass a size hint to optimize the fast path for their case, and make that template reusable for all the intrinsics. That would be nice. But for now, I can just go for a retval check and fallback slow path, and maybe dress it up to a template in a future PReq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, check out the new version. Also, I just noticed that the get_action implementation has a memory leak in the greater than 512 byte case.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nathanielhourt PR has been approved. Thanks. Do you want to add something or just letting enf dev merge by themselves? normally developer pushes their changes so you are good to go if you are done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I don't have privs to merge it even after approval. I figured someone else would do it. =)

char* struct_buffer = (char*)alloca(struct_buffer_size);

using VersionType = decltype(code_hash_result::struct_version);
const VersionType STRUCT_VERSION = 0;
internal_use_do_not_use::get_code_hash(account.value, STRUCT_VERSION, struct_buffer, struct_buffer_size);
check(unpack<VersionType>(struct_buffer, struct_buffer_size) == STRUCT_VERSION,
"Hypervisor returned unexpected code hash struct version");
unpack(*full_result, struct_buffer, struct_buffer_size);

return full_result->code_hash;
}

/**
* Copy up to length bytes of current action data to the specified location
*
Expand Down
5 changes: 5 additions & 0 deletions tests/integration/contracts.hpp.in
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,10 @@ namespace eosio::testing {

static std::vector<uint8_t> crypto_primitives_test_wasm() { return read_wasm("${CMAKE_BINARY_DIR}/../unit/test_contracts/crypto_primitives_tests.wasm"); }
static std::vector<char> crypto_primitives_test_abi() { return read_abi("${CMAKE_BINARY_DIR}/../unit/test_contracts/crypto_primitives_tests.abi"); }

static std::vector<uint8_t> get_code_hash_write_test_wasm() { return read_wasm("${CMAKE_BINARY_DIR}/../unit/test_contracts/get_code_hash_write.wasm"); }
static std::vector<char> get_code_hash_write_test_abi() { return read_abi("${CMAKE_BINARY_DIR}/../unit/test_contracts/get_code_hash_write.abi"); }
static std::vector<uint8_t> get_code_hash_read_test_wasm() { return read_wasm("${CMAKE_BINARY_DIR}/../unit/test_contracts/get_code_hash_read.wasm"); }
static std::vector<char> get_code_hash_read_test_abi() { return read_abi("${CMAKE_BINARY_DIR}/../unit/test_contracts/get_code_hash_read.abi"); }
};
} //ns eosio::testing
46 changes: 46 additions & 0 deletions tests/integration/get_code_hash_tests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
#include <boost/test/unit_test.hpp>
#include <eosio/testing/tester.hpp>
#include <eosio/chain/abi_serializer.hpp>

#include <Runtime/Runtime.h>

#include <fc/variant_object.hpp>

#include <contracts.hpp>

using namespace eosio;
using namespace eosio::testing;
using namespace eosio::chain;
using namespace fc;

using mvo = fc::mutable_variant_object;

struct code_hash {
uint64_t id;
fc::sha256 hash;
uint64_t primary_key() const { return id; }
};
FC_REFLECT(code_hash, (id)(hash))

BOOST_AUTO_TEST_SUITE(get_code_hash_tests_suite)

BOOST_FIXTURE_TEST_CASE( get_code_hash_tests, tester ) try {
create_accounts( { "test"_n } );
produce_block();

set_code( "test"_n, contracts::get_code_hash_write_test_wasm() );
set_abi( "test"_n, contracts::get_code_hash_write_test_abi().data() );

produce_blocks();
push_action("test"_n, "theaction"_n, "test"_n, mvo());
code_hash entry;
get_table_entry(entry, "test"_n, "test"_n, "code.hash"_n, 0);
wdump((entry.hash));

set_code( "test"_n, contracts::get_code_hash_read_test_wasm() );
produce_blocks();

push_action("test"_n, "theaction"_n, "test"_n, mvo());
} FC_LOG_AND_RETHROW()

BOOST_AUTO_TEST_SUITE_END()
2 changes: 2 additions & 0 deletions tests/unit/test_contracts/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ add_contract(explicit_nested_tests explicit_nested_tests explicit_nested_tests.c
add_contract(transfer_contract transfer_contract transfer.cpp)
add_contract(minimal_tests minimal_tests minimal_tests.cpp)
add_contract(crypto_primitives_tests crypto_primitives_tests crypto_primitives_tests.cpp)
add_contract(get_code_hash_tests get_code_hash_write get_code_hash_write.cpp)
add_contract(get_code_hash_tests get_code_hash_read get_code_hash_read.cpp)
add_contract(capi_tests capi_tests capi/capi.c capi/action.c capi/chain.c capi/crypto.c capi/db.c capi/permission.c
capi/print.c capi/privileged.c capi/system.c capi/transaction.c)

Expand Down
1 change: 1 addition & 0 deletions tests/unit/test_contracts/capi/action.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ void test_action( void ) {
send_context_free_inline(NULL, 0);
publication_time();
current_receiver();
get_code_hash(0, 0, NULL, 0);
set_action_return_value(NULL, 0);
}
27 changes: 27 additions & 0 deletions tests/unit/test_contracts/get_code_hash_read.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#include <eosio/eosio.hpp>
#include <eosio/action.hpp>
#include <eosio/name.hpp>

#include "get_code_hash_table.hpp"

class [[eosio::contract]] get_code_hash_tests : public contract {
public:
using contract::contract;

using hash_table = multi_index<name("code.hash"), code_hash>;

// Read the old code's hash from database and verify new code's hash differs
[[eosio::action]]
void theaction() {
require_auth(get_self());
hash_table hashes(get_self(), get_self().value);

auto hash = get_code_hash(get_self());
check(hash != checksum256(), "Code hash should not be null");

auto record = hashes.get(0, "Unable to find recorded hash");
check(hash != record.hash, "Code hash has not changed");
eosio::print("Old hash: ", record.hash, "; new hash: ", hash);
}
};

10 changes: 10 additions & 0 deletions tests/unit/test_contracts/get_code_hash_table.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#pragma once

using namespace eosio;

TABLE code_hash {
uint64_t id;
checksum256 hash;
uint64_t primary_key() const { return id; }
};

28 changes: 28 additions & 0 deletions tests/unit/test_contracts/get_code_hash_write.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#include <eosio/eosio.hpp>
#include <eosio/action.hpp>
#include <eosio/name.hpp>

#include "get_code_hash_table.hpp"

class [[eosio::contract]] get_code_hash_tests : public contract {
public:
using contract::contract;

using hash_table = multi_index<name("code.hash"), code_hash>;

// Write this code's hash to database
[[eosio::action]]
void theaction() {
require_auth(get_self());
hash_table hashes(get_self(), get_self().value);

auto hash = get_code_hash(get_self());
check(hash != checksum256(), "Code hash should not be null");

hashes.emplace(get_self(), [&hash](auto& t) {
t.id = 0;
t.hash = hash;
});
}
};