debug_accountRangeAt #4987

winsvega · 2018-04-27T20:55:32Z

Debug account range at.

Get the list of accounts in the given block after given transaction index with provided account mask.

chfast · 2018-04-27T21:12:25Z

libweb3jsonrpc/Debug.cpp

+	{
+		Block block = m_eth.block(blockHash(_blockHashOrNumber));
+
+		unsigned const i = ((unsigned)_txIndex < block.pending().size()) ? (unsigned)_txIndex : block.pending().size();


Use static_cast for casting and do it once.

check Debug::debug_storageRangeAt below this method

Copy-pasting bad code is still bad code.

I mean should I fix that part too I suppose I should.

You can but that's not required.
Here I'd do:

size_t const i = std::min(static_cast<size_t>(_txIndex), block.pending().size());

But I'm not sure this is good logic. Shouldn't we report error in case the transaction index is wrong? And txIndex == block.pending().size() seems to be also wrong.

chfast · 2018-04-27T21:14:47Z

libweb3jsonrpc/Debug.cpp

+		unsigned const i = ((unsigned)_txIndex < block.pending().size()) ? (unsigned)_txIndex : block.pending().size();
+		State state(State::Null);
+		createIntermediateState(state, block, i, m_eth.blockChain());
+		Address from(_address);


If you use const for local variables, do it everywhere.

chfast · 2018-04-27T21:15:29Z

libweb3jsonrpc/Debug.cpp

+		State state(State::Null);
+		createIntermediateState(state, block, i, m_eth.blockChain());
+		Address from(_address);
+		for (auto const& addr : state.addresses())


This does not seem to be efficient way of filtering accounts.

so the better way I see would be to sort addresses first then just select address

Not at all. The problem is you have to load all accounts from database first. That's not a big deal in test chain but on the mainnet that's not feasible. You have to extend State interface to support returning a subset of addresses.

Also it should return hashes of addresses, not addresses, because addresses are not always available (e.g. not available after snapshot sync)

@cdetrio also mentioned it in ethereum/retesteth#5 (comment)

hmm. not sure what I would do with address hashes. if this function return list of hashes I wont be able to tell which address is unexpectedly exist in the post state.

Optional is fine. I don't get how @chfast wants to optimize and select required addresses from m_state without iterating. is m_state ordered?

Sure it's ordered, it's a trie. I think you should utilize lower_bound method of the Trie classes. Somewhat similar to how debug_storageRangeAt finds the point to start iterating.

@gumb0 The key in the database is the hash of the address? so we cannot filter the addresses without first loading it into the cache?

@chfast I think we can, we'll just have a special method for this in State, that will not update m_cache, but use lower-level feature of TrieDB

It's similar to State::storage(Address const& _id) (which is used for debug_storageRangeAt) - it iterates over the storage directly from the database, storage cache is not updated

That was my intention here from the beginning.

codecov-io · 2018-04-27T23:11:43Z

Codecov Report

Merging #4987 into develop will increase coverage by 0.02%.
The diff coverage is 78.12%.

@@             Coverage Diff             @@
##           develop    #4987      +/-   ##
===========================================
+ Coverage    59.74%   59.76%   +0.02%     
===========================================
  Files          346      346              
  Lines        26685    26870     +185     
  Branches      3188     3222      +34     
===========================================
+ Hits         15943    16060     +117     
- Misses        9692     9738      +46     
- Partials      1050     1072      +22

gumb0 · 2018-05-08T13:43:15Z

Now you should be able to define a method in State something like

pair<map<h256, Address>,h256> State::addresses(h256 const& _begin, int _maxResults) const
{
    map<h256, Address> ret;
    h256 nextKey;


    for (auto it = m_state.hashedLowerBound(_begin); it != m_state.hashedEnd(); ++it)
    {
        if (ret.size() == _maxResults)
        {
           nextKey = (*it).first;
           break;
        }

        h256 const hashedAddress((*it).first);
        Address const address = Address(it.key());
        // maybe here check in m_cache whether account is still alive
        ret[hashedAddress] = address;
    }

    return {ret, nextKey};
}

gumb0 · 2018-05-18T13:28:38Z

The problem with my code there ^ discovered.
We should take into account m_cache better, that is some account could be created but exist only in m_cache, not in m_state. We should merge them somehow, but m_cache is ordered by Address while m_state by hash of address...

…ash of the key

chfast · 2018-05-22T09:14:45Z

libweb3jsonrpc/Debug.cpp

+        Block block = m_eth.block(blockHash(_blockHashOrNumber));
+        size_t const i = std::min(static_cast<size_t>(_txIndex), block.pending().size());
+        State state(State::Null);
+        if (block.info().number() >= m_eth.chainParams().byzantiumForkBlock)


Shouldn't this be the other way around? We have to create intermediate state in Byzantium+, right?

createIntermediateState works for both cases (creates state from intermediate root when available, executes previous transactions when not)
Then State::addresses() doesn't work right for this state with re-executed transactions (the result of execution is in cache and it doesn't take into account cache)

This if shouldn't be here and State::addresses() should be fixed, I'm going to do it

chfast · 2018-05-22T09:15:26Z

libethereum/State.h


+    /// @returns the map with maximum _maxResults elements containing hash->addresses and the next
+    /// address hash. This method faster then addresses() const;
+    typedef std::map<h256, Address> addressMap;


Change to using AddressMap = std::map<h256, Address>;

chfast · 2018-05-22T09:15:54Z

libethereum/State.cpp


+std::pair<State::addressMap, h256> State::addresses(h256 const& _begin, size_t _maxResults) const
+{
+    map<h256, Address> ret;


Use AddressMap type.

gumb0 · 2018-05-22T12:36:53Z

libweb3jsonrpc/Debug.cpp

+        Block block = m_eth.block(blockHash(_blockHashOrNumber));
+        size_t const i = std::min(static_cast<size_t>(_txIndex), block.pending().size());
+        State state(State::Null);
+        if (block.info().number() >= m_eth.chainParams().byzantiumForkBlock)


createIntermediateState works for both cases (creates state from intermediate root when available, executes previous transactions when not)
Then State::addresses() doesn't work right for this state with re-executed transactions (the result of execution is in cache and it doesn't take into account cache)

This if shouldn't be here and State::addresses() should be fixed, I'm going to do it

@winsvega

@winsvega Please check how it now works for you

winsvega · 2018-05-23T13:04:35Z

works

*** No errors detected
Total Time:                                       : 490               
stStaticCall                                  time: 85.592            
stTransactionTest                             time: 60.4297           
stPreCompiledContracts                        time: 59.8086           
stZeroKnowledge                               time: 57.9728           
stBadOpcode                                   time: 53.135            
stStackTests                                  time: 20.6821           
stRevertTest                                  time: 20.5036           
stRandom                                      time: 15.6341           
stCallCodes                                   time: 13.4573           
stPreCompiledContracts2                       time: 12.5582           
stRandom2                                     time: 10.5036           
stZeroKnowledge2                              time: 9.31869

I still think there should be a way for optimization of eth calls.
and cpp client in general.

A simple transaction import runs the VM execution twice.

upon transaction import (send raw transaction)
when mining a block.

chfast · 2018-05-23T13:40:35Z

This is ok, but AppVeyor fails to test within 1 hour.

chfast · 2018-05-23T19:04:48Z

Could the performance degraded because of the changes? AppVeyor is on the edge now.

winsvega · 2018-05-23T20:00:13Z

unit test could have added some to execution time I think.

gumb0 · 2018-05-24T08:30:58Z

@chfast No, new unit tests are quick I think. There's one of the old tests that hangs sometimes #5038 I'll fix it

winsvega requested review from gumb0 and pirapira April 27, 2018 21:02

chfast suggested changes Apr 27, 2018

View reviewed changes

winsvega and others added 3 commits May 18, 2018 20:06

debug_accountRangeAt

285ad5e

Add possibility to FatGenericTrieDB to start iteration with a given h…

1daf355

…ash of the key

state addresses ordered by hash

abcfc3f

winsvega force-pushed the smallrpc2 branch from ce9b9c1 to abcfc3f Compare May 18, 2018 17:19

chfast suggested changes May 22, 2018

View reviewed changes

gumb0 previously requested changes May 22, 2018

View reviewed changes

gumb0 added 2 commits May 22, 2018 17:07

Fix State::addresses() to correctly take into account m_cache

ec1f86b

Remove unneeded Byzantium workaround from debug_accountRangeAt

b9c22f8

gumb0 approved these changes May 23, 2018

View reviewed changes

chfast approved these changes May 23, 2018

View reviewed changes

gumb0 merged commit aacde6e into develop May 24, 2018

gumb0 deleted the smallrpc2 branch May 24, 2018 17:38

debug_accountRangeAt #4987

debug_accountRangeAt #4987

Uh oh!

Conversation

winsvega commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumb0 Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gumb0 commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gumb0 commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winsvega commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chfast commented May 23, 2018

Uh oh!

chfast commented May 23, 2018

Uh oh!

winsvega commented May 23, 2018

Uh oh!

gumb0 commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

winsvega commented Apr 27, 2018 •

edited

Loading

gumb0 Apr 30, 2018 •

edited

Loading

codecov-io commented Apr 27, 2018 •

edited

Loading

gumb0 commented May 8, 2018 •

edited

Loading

winsvega commented May 23, 2018 •

edited

Loading