(BIDS-2634) reordered transactions, fix almost-empty table #2663

remoterami · 2023-11-03T10:49:20Z

`🤖 Generated by Copilot at a7a7b97`

This pull request fixes some issues with the eth1 transactions API and the Bigtable database query. It improves the pagination and order of the eth1Transactions.go endpoint and prevents an error in the GetBlocksDescending function of bigtable_eth1.go.

D13ce

The transaction table on the address page does not seem to be affected by your changes.

Here is a random example I have found:
https://holesky.beaconcha.in/address/0x6cc9397c3b38739dacbfaa68ead5f5d77ba5f455

=>
https://holesky.beaconcha.in/block/273427#transactions

So right now, the txs shown in the tx table on the address page are sorted by block in descending order (i.e. txs in younger blocks are listed ABOVE txs in older blocks) and when there are more txs in one block, lower index tx come first. Your PR should change the latter so that higher indexes are listed below, right? I am asking because of your change here: https://github.com/gobitfly/eth2-beaconchain-explorer/pull/2663/files#diff-143a07e5aa31442d87e5dd9584d1525fc98e8b2fef161a08af276d65f60184acR2137-R2138

If I run your branch and navigate to http://localhost:8081/address/0x6cc9397c3b38739dacbfaa68ead5f5d77ba5f455, it looks the same though:

Please share your thoughts.

FYI, I have not tested the other tx tables on the address page as it is possible that I am misunderstanding your intentions.

remoterami · 2023-11-09T10:00:44Z

Oh I missed that, great catch. Iterating in reverse order doesn't change anything if we update by index, my bad 😅 Just swapping the insert order won't be enough anyways though if there are more than 25 transactions by one address per block. Because of the way the data is retrieved from bigtable, it wouldn't be sorted anymore at all - indices would be sorted in the interval, but there'd be jumps between intervals. This does also occur in pratice, e.g. with every big NFT mint there'll be >25 transactions per block. I'll try to come up with something, hopefully it's not too inefficient..

EDIT: Disregard this, I wasn't able to recall/reproduce what this was actually about after coming back to this. The new implementation regarding the paragraph below should address and hopefully fix your concern.

And this also made me aware of another issue with reversePaddedIndex - it doesn't do padding correctly, resulting in index 0 being larger than index 1. This currently results in index 0 always being at the top of the list, immediately followed by the max index.

Eisei24

Everything looks correct now according to my testing but please confirm that I understood the issues we have on master correctly:

For the /transactions page the blocks were correctly descending but all the tx were ascending so the tx order needed to be reverted
For the /address page both blocks and tx were already correctly descending, however there was a bug that the first tx (idx 0) was always incorrectly at the top.

Eisei24 · 2024-01-30T16:13:32Z

db/bigtable_eth1.go

+	}
+
+	// first find out if we've a (sub)transaction with index 0 whose block/transaction has maybe not been completed by the request
+	// if we find one, make sure we do. So we won't miss that (i)tx next time (=> ignoring the query limit)


minor: The comment seems incomplete.

Eisei24 · 2024-01-30T16:18:39Z

db/bigtable_eth1.go

+	}
+	// tx idx
+	if i_splits[6] != j_splits[6] {
+		if i_splits[6] == "10000" || j_splits[6] == "10000" {


note: Keep in mind that with BIDS-2550 we added TX_PER_BLOCK_LIMIT and ITX_PER_TX_LIMIT.
When some kind of merging happens these should be used throughout the new methods instead of the hardcoded "10000"/"100000".

Yes I'm aware and intended to take care of this when necessary, just not possible yet when we don't know which PR will be merged first

remoterami · 2024-01-31T11:41:52Z

Yes, your understanding is correct 👍

Eisei24

lgtm

D13ce

As discussed with @Eisei24 , my initial concerns have been properly tackled.

…ect position

Eisei24

lgtm, error message could still be updated.

Eisei24 · 2024-02-06T12:24:06Z

db/bigtable_eth1.go

@@ -2073,7 +2073,7 @@ func skipBlockIfLastTxIndex(key string) string {
 	return key
 }

-func (bigtable *Bigtable) GetEth1TxForAddress(prefix string, limit int64) ([]*types.Eth1TransactionIndexed, string, error) {
+func (bigtable *Bigtable) GetEth1TxsForAddress(prefix string, limit int64) ([]*types.Eth1TransactionIndexed, []string, error) {


minor: This is just a change from a merge but the method name changed here but not in the error message here

eth2-beaconchain-explorer/db/bigtable_eth1.go

Line 2123 in 8b8e6e6

logger.WithError(err).WithField("prefix", prefix).WithField("limit", limit).Errorf("error reading rows in bigtable_eth1 / GetEth1TxForAddress")

Eisei24

And again, lgtm.

D13ce suggested changes Nov 8, 2023

View reviewed changes

remoterami force-pushed the BIDS-2634/reverse_iterate_transactions branch from 552fabe to 80c668e Compare December 19, 2023 12:16

Eisei24 reviewed Jan 30, 2024

View reviewed changes

Eisei24 previously approved these changes Jan 31, 2024

View reviewed changes

D13ce previously approved these changes Jan 31, 2024

View reviewed changes

remoterami added 7 commits February 6, 2024 12:35

(BIDS-2634) reordered transactions, fix almost-empty table

d708951

(BIDS-2634) reverse sorting for /address tables

d84dad7

(BIDS-2634) workaround: sort misplaced reversePaddedIndex 0 into corr…

76cde1e

…ect position

(BIDS-2634) handle trace position 0

06ff6fe

(BIDS-2634) changed comment

01a68bb

(BIDS-2634) return all index keys

165238f

(BIDS-2634) using constant (i)tx limits

8b8e6e6

remoterami dismissed stale reviews from D13ce and Eisei24 via 8b8e6e6 February 6, 2024 12:13

remoterami force-pushed the BIDS-2634/reverse_iterate_transactions branch from 3b9e3a7 to 8b8e6e6 Compare February 6, 2024 12:13

(BIDS-2634) fixed last key handling

1550f37

Eisei24 previously approved these changes Feb 6, 2024

View reviewed changes

(BIDS-2634) adjusted error message

04fb19a

remoterami dismissed Eisei24’s stale review via 04fb19a February 6, 2024 13:19

Eisei24 approved these changes Feb 6, 2024

View reviewed changes

remoterami merged commit 9d21f02 into master Feb 6, 2024
3 checks passed

remoterami deleted the BIDS-2634/reverse_iterate_transactions branch February 6, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(BIDS-2634) reordered transactions, fix almost-empty table #2663

(BIDS-2634) reordered transactions, fix almost-empty table #2663

remoterami commented Nov 3, 2023 •

edited by ghost

Loading

D13ce left a comment

remoterami commented Nov 9, 2023 •

edited

Loading

Eisei24 left a comment

Eisei24 Jan 30, 2024

Eisei24 Jan 30, 2024

remoterami Jan 31, 2024

remoterami commented Jan 31, 2024

Eisei24 left a comment

D13ce left a comment

Eisei24 left a comment

Eisei24 Feb 6, 2024

Eisei24 left a comment

(BIDS-2634) reordered transactions, fix almost-empty table #2663

(BIDS-2634) reordered transactions, fix almost-empty table #2663

Conversation

remoterami commented Nov 3, 2023 • edited by ghost Loading

🤖 Generated by Copilot at a7a7b97

D13ce left a comment

Choose a reason for hiding this comment

remoterami commented Nov 9, 2023 • edited Loading

Eisei24 left a comment

Choose a reason for hiding this comment

Eisei24 Jan 30, 2024

Choose a reason for hiding this comment

Eisei24 Jan 30, 2024

Choose a reason for hiding this comment

remoterami Jan 31, 2024

Choose a reason for hiding this comment

remoterami commented Jan 31, 2024

Eisei24 left a comment

Choose a reason for hiding this comment

D13ce left a comment

Choose a reason for hiding this comment

Eisei24 left a comment

Choose a reason for hiding this comment

Eisei24 Feb 6, 2024

Choose a reason for hiding this comment

Eisei24 left a comment

Choose a reason for hiding this comment

remoterami commented Nov 3, 2023 •

edited by ghost

Loading

`🤖 Generated by Copilot at a7a7b97`

remoterami commented Nov 9, 2023 •

edited

Loading