feat: return blknum and transaction index on transaction submit #133

pgebal · 2020-09-22T17:39:53Z

Returns block number an index for a submitted transaction.

To determine transaction index on submission I had to lock transaction on currently forming block.
Changes:

transactions are associated with block and have index set on insert
position is assigned to output on insert
Engine.DB.Transaction.insert inserts a transaction and new block if current forming block exceeds transaction limit
Engine.DB.Block.form no longer inserts a block. It calculates merkle root hash and sets block state to pending_submission.
Engine.DB.Transaction.insert required that there is a forming block when inserting a transaction
refactor: separated transaction query, transaction changeset and block query modules
assumes that there is always a pending block in the database

apps/engine/lib/engine/callbacks/in_flight_exit_started.ex

apps/engine/lib/engine/callbacks/piggyback.ex

apps/engine/lib/engine/db/block.ex

apps/engine/lib/engine/db/output.ex

apps/engine/lib/engine/db/transaction.ex

apps/engine/lib/engine/db/block.ex

apps/engine/test/engine/db/transaction_test.exs

apps/engine/test/engine/db/block_test.exs

pgebal · 2020-09-28T16:10:15Z

apps/engine/lib/engine/db/block.ex

-    |> Multi.run("new-block", &insert_block/2)
-    |> Multi.run("form-block", &attach_transactions_to_block/2)
-    |> Multi.run("hash-block", &generate_block_hash/2)
+    |> Multi.run("block", &get_or_insert_forming_block/2)


I think that has to be used carefully.
If there is no forming block, then each concurrent transaction could insert a block.
That should not be the case becausewe won't be probably forming empty blocks and after each inserting a transaction via Transaction.insert we'll always have a forming block in the db.

apps/engine/lib/engine/db/transaction.ex

apps/engine/lib/engine/db/block.ex

pgebal · 2020-09-29T17:06:57Z

apps/engine/lib/engine/db/block.ex

+    # basic version, checking transaction limit is postponed until we get transction fees implemented
+    case last_tx_index >= @max_transaction_in_block do
+      true ->
+        {:ok, _} = prepare_for_submission(repo, %{"block" => block})


As @InoMurko pointed in discussion in the issue - this should not be done on insert, especially calculating block's hash.

Yeah and you are missing fee transactions. This should just transition the block to a new state so it can be picked by an other service to process the finalization (add fee tx / calculate hash and stuff).

apps/api/lib/api/v1/controllers/transaction_controller.ex

apps/api/test/api/v1/controllers/transaction_controller_test.exs

apps/api/test/api/v1/router_test.exs

apps/engine/test/engine/db/transaction_test.exs

pgebal · 2020-10-06T16:53:14Z

apps/engine/test/engine/db/block_test.exs

      assert block.hash == hash
    end

    test "correctly generates block hash for empty txs list" do
-      assert {:ok, %{"hash-block" => block}} = Block.form()
+      assert {:ok, %{block_for_submission: block}} = Block.form()


Shouldn't we disallow empty blocks? Empty blocks have the same hash.

yes! we shouldn't form empty blocks, but I guess this can be done in your next PR when you tackle the block finalization?

apps/engine/lib/engine/db/transaction/transaction_changeset.ex

apps/engine/test/engine/db/transaction/transaction_async_test.exs

mederic-p · 2020-10-07T04:13:36Z

apps/engine/test/engine/db/transaction/transaction_async_test.exs

+      assert tx1.tx_index + 1 == tx2.tx_index
+    end
+
+    test "does not create conflicts when inserting multiple transaction concurrently" do


I thought that only this test was causing an issue when using async: true because it was doing async stuff.
Other tests in this file could go to the other test file that runs with async: true, no?

I thought that too, but there was 1 more insert test failing, so I decided to move them all here. I think we do not have to be concerned much about those tests running long, and there is a benefit of having all insert tests in one place.

mederic-p · 2020-10-07T04:15:26Z

apps/engine/test/engine/db/block_test.exs

      assert block.hash == hash
    end

    test "correctly generates block hash for empty txs list" do
-      assert {:ok, %{"hash-block" => block}} = Block.form()
+      assert {:ok, %{block_for_submission: block}} = Block.form()


yes! we shouldn't form empty blocks, but I guess this can be done in your next PR when you tackle the block finalization?

apps/engine/lib/engine/db/block/block_changeset.ex

apps/engine/lib/engine/db/block.ex

apps/api/test/api/v1/controllers/transaction_controller_test.exs

apps/api/test/api/v1/router_test.exs

apps/engine/lib/engine/db/block.ex

apps/engine/lib/engine/db/block/block_query.ex

InoMurko · 2020-10-07T10:57:59Z

apps/engine/lib/engine/db/transaction.ex

+        {:error, changeset}
+
+      error ->
+        _ = Logger.error("Error when inserting transaction #{inspect(error)}")


this would be a good datadog counter

apps/engine/lib/engine/db/transaction.ex

apps/engine/lib/engine/db/transaction/transaction_query.ex

InoMurko · 2020-10-07T11:11:34Z

apps/engine/lib/engine/db/transaction.ex

+    |> Multi.run(:current_forming_block, &Block.get_forming_block_for_update/2)
+    |> Multi.run(:block_with_next_tx_index, &Block.get_block_and_tx_index_for_transaction/2)


it seems to me that perhaps these two reads could be just one.
get the block and it's max transaction index:

last_tx_index = repo.one(TransactionQuery.select_max_tx_index_for_block(block.id)) || -1

https://www.postgresqltutorial.com/postgresql-inner-join/#:~:text=To%20select%20data%20from%20both,data%20in%20the%20SELECT%20clause.

That's a good idea to check if it improves performance. I'll dig into this and if it speeds things up, I'll create a separate PR for that.

apps/engine/test/support/db/factory.ex

mederic-p · 2020-10-08T05:02:01Z

apps/engine/lib/engine/db/block.ex

-    |> changeset(params)
-    |> repo.insert()
+    |> BlockChangeset.new_block_changeset(params)
+    |> repo.insert(on_conflict: :nothing)


Don't we have a problem when this conflict happens? Last time I tried, the block returned didn't have an id so the transaction was failing. I could fix it by adding a

|> case do {:ok, %{id: nil} -> re-query pending forming block until we get iit. .....

But I don't know if it's a valid solution.

this would happen if a block is inserted by another transaction, right?

this method is called only when:

Block.formed -> we insert a new forming block, we can do nothing on conflict, forming block is in the database

Transaction.insert -> we insert a new block which we will associate a transaction with. In that case always Block.get_or_insert_forming_block is called, which selects a forming block from db after insert.

So it should be fine.

pgebal · 2020-10-08T18:22:01Z

apps/engine/test/engine/db/transaction_test.exs

@@ -1,98 +1,102 @@
 defmodule Engine.DB.TransactionTest do
-  use Engine.DB.DataCase, async: true
+  use Engine.DB.DataCase, async: false


This is done on purpose
I think all tests using Shared mode Sandbox should not be run sync.
https://hexdocs.pm/ecto_sql/Ecto.Adapters.SQL.Sandbox.html#module-shared-mode

_So Ecto.Adapters.SQL.Sandbox provides :shared mode, which a process can switch to after checkout/1. Ecto.Adapters.SQL.Sandbox.mode(Repo, {:shared, pid}) means “all processes running concurrently with pid should use the connection which pid owns.”

As the docs for SQL.Sandbox say, using :shared mode means that “tests can no longer run concurrently.” With the understanding we’ve built, this makes sense. If shared mode means “all concurrent processes share this connection,” we only want one one test process to be in that concurrent group. If any other test ran at the same time, the two tests would be working with the database within the same transaction and the changes that one test made would leak to the other test. This would destroy our illusion that each test has a database to itself.

So any test using :shared mode must be in a test module marked async: false. This means no tests from other modules will run concurrently with it. Since tests in the same module run serially, it will be the only test running until it completes and may share its database connection fearlessly with the processes whose behavior it tests._

yeah, seems correct!

mederic-p

Great job! Let's start with this and see how we can improve it later.

mederic-p · 2020-10-09T04:00:24Z

apps/engine/lib/engine/db/transaction.ex

@@ -51,7 +51,7 @@ defmodule Engine.DB.Transaction do
          tx_hash: <<_::256>>,
          tx_type: pos_integer(),
          updated_at: DateTime.t(),
-          witnesses: DateTime.t()


mederic-p · 2020-10-09T04:01:20Z

apps/engine/lib/engine/db/transaction.ex

  """
+  def insert(tx_bytes) do


Cool, I think it makes more sense like this!

…tting a transaction

pgebal force-pushed the pgebal/draft-block-forming branch 3 times, most recently from 3fcb21d to 0b542b2 Compare September 23, 2020 14:31

mederic-p suggested changes Sep 24, 2020

View reviewed changes

pgebal force-pushed the pgebal/draft-block-forming branch 2 times, most recently from a3ff177 to 3d9b6a8 Compare September 24, 2020 17:34

mederic-p reviewed Sep 25, 2020

View reviewed changes

apps/engine/test/engine/db/block_test.exs Outdated Show resolved Hide resolved

pgebal force-pushed the pgebal/draft-block-forming branch 9 times, most recently from 41e08e1 to 620e3b0 Compare September 28, 2020 15:57

pgebal changed the title ~~WIP - calculating tx index on transaction insert~~ feat: return blknum and transaction index on transaction submit Sep 28, 2020

pgebal marked this pull request as ready for review September 28, 2020 16:01

pgebal commented Sep 28, 2020

View reviewed changes

pgebal force-pushed the pgebal/draft-block-forming branch from 620e3b0 to 1f13a7b Compare September 28, 2020 16:19

pgebal requested review from mederic-p and achiurizo September 28, 2020 16:22

mederic-p suggested changes Sep 29, 2020

View reviewed changes

apps/engine/lib/engine/db/transaction.ex Outdated Show resolved Hide resolved

mederic-p reviewed Sep 29, 2020

View reviewed changes

apps/engine/lib/engine/db/block.ex Outdated Show resolved Hide resolved

pgebal mentioned this pull request Sep 29, 2020

Implement inserting transactions into a forming block #131

Closed

InoMurko reviewed Sep 29, 2020

View reviewed changes

apps/engine/lib/engine/db/block.ex Outdated Show resolved Hide resolved

pgebal commented Sep 29, 2020

View reviewed changes

pgebal force-pushed the pgebal/draft-block-forming branch from 16a708c to 075e989 Compare October 1, 2020 16:33

pgebal requested review from mederic-p and InoMurko October 5, 2020 17:48

mederic-p suggested changes Oct 6, 2020

View reviewed changes

pgebal commented Oct 6, 2020

View reviewed changes

mederic-p reviewed Oct 7, 2020

View reviewed changes

mederic-p suggested changes Oct 7, 2020

View reviewed changes

apps/engine/lib/engine/db/block/block_changeset.ex Show resolved Hide resolved

apps/engine/lib/engine/db/block.ex Outdated Show resolved Hide resolved

InoMurko approved these changes Oct 7, 2020

View reviewed changes

mederic-p reviewed Oct 8, 2020

View reviewed changes

pgebal commented Oct 8, 2020

View reviewed changes

mederic-p approved these changes Oct 9, 2020

View reviewed changes

pgebal force-pushed the pgebal/draft-block-forming branch from 14fcee5 to 5e648a3 Compare October 9, 2020 12:02

feat: return block number and transaction index in response for submi…

47c8215

…tting a transaction

pgebal force-pushed the pgebal/draft-block-forming branch from c23be95 to 47c8215 Compare October 9, 2020 15:04

pgebal merged commit 9d8352b into master Oct 9, 2020

pgebal deleted the pgebal/draft-block-forming branch October 9, 2020 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: return blknum and transaction index on transaction submit #133

feat: return blknum and transaction index on transaction submit #133

pgebal commented Sep 22, 2020 •

edited

Loading

pgebal Sep 28, 2020

pgebal Sep 29, 2020

mederic-p Oct 1, 2020

pgebal Oct 6, 2020

mederic-p Oct 7, 2020

pgebal Oct 7, 2020

mederic-p Oct 7, 2020

pgebal Oct 7, 2020 •

edited

Loading

mederic-p Oct 7, 2020

InoMurko Oct 7, 2020

InoMurko Oct 7, 2020

pgebal Oct 7, 2020

mederic-p Oct 8, 2020

InoMurko Oct 8, 2020

pgebal Oct 8, 2020 •

edited

Loading

pgebal Oct 8, 2020

InoMurko Oct 8, 2020 •

edited

Loading

mederic-p left a comment

mederic-p Oct 9, 2020

mederic-p Oct 9, 2020

		\|> Multi.run(:current_forming_block, &Block.get_forming_block_for_update/2)
		\|> Multi.run(:block_with_next_tx_index, &Block.get_block_and_tx_index_for_transaction/2)

feat: return blknum and transaction index on transaction submit #133

feat: return blknum and transaction index on transaction submit #133

Conversation

pgebal commented Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgebal Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgebal Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InoMurko Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

mederic-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgebal commented Sep 22, 2020 •

edited

Loading

pgebal Oct 7, 2020 •

edited

Loading

pgebal Oct 8, 2020 •

edited

Loading

InoMurko Oct 8, 2020 •

edited

Loading