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

Batch sn payments #1450

Merged
merged 3 commits into from
May 10, 2022
Merged

Batch sn payments #1450

merged 3 commits into from
May 10, 2022

Conversation

darcys22
Copy link
Collaborator

No description provided.

@majestrate
Copy link

this will be a chonky PR when it's ready

@darcys22 darcys22 force-pushed the batch-sn-payments branch from 9e683e3 to 3d41efe Compare April 29, 2021 06:01
@darcys22 darcys22 force-pushed the batch-sn-payments branch 3 times, most recently from c196b19 to f46efa2 Compare May 18, 2021 00:19
@darcys22 darcys22 force-pushed the batch-sn-payments branch from f46efa2 to 7ade8b2 Compare May 20, 2021 04:23
@darcys22
Copy link
Collaborator Author

darcys22 commented Jun 25, 2021

Todo:

  • Tests
  • switch sqlite_orm to another lib
  • use trigger to clear out zero amount balances
  • constrain sqlite table to only allow positive balances
  • check functionality of subaddresses
  • mining after SN network running causes miner to crash

Trigger

CREATE TRIGGER batch_payments_delete_empty
AFTER UPDATE ON batch_payments FOR EACH ROW WHEN NEW.amount = 0 THEN DELETE FROM batch_payments WHERE address = NEW.address;

Constraint

CHECK(amount >= 0)

@jagerman
Copy link
Member

switch sqlite_orm to another lib

-> https://github.com/SRombauts/SQLiteCpp now used in storage server, lets you write your own queries but keeps a C++ interface that's much nicer to work with than the straight C interface.

@darcys22 darcys22 force-pushed the batch-sn-payments branch 2 times, most recently from a6b57ea to 45f7c07 Compare July 1, 2021 00:56
@darcys22 darcys22 force-pushed the batch-sn-payments branch 2 times, most recently from de43b6b to 997039a Compare July 9, 2021 03:00
cmake/CMakeDoxyfile.in Outdated Show resolved Hide resolved
@darcys22
Copy link
Collaborator Author

darcys22 commented Aug 2, 2021

Misc other issues:

  • Clean up CMake folder
  • Rebase
  • Simulate batching and decide on final length of batching period

@darcys22 darcys22 force-pushed the batch-sn-payments branch 3 times, most recently from f20d57e to 28c6d84 Compare August 23, 2021 00:34
CMakeLists.txt Outdated
@@ -86,6 +86,9 @@ else()
message(WARNING "PIE disabled: cmake 3.14+ is required for proper PIE linking support")
endif()

# TODO sean remove this
set(CMAKE_CXX_LINK_EXECUTABLE "${CMAKE_CXX_LINK_EXECUTABLE} -ldl")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was something that came up when trying to run cmake with -DCMAKE_BUILD_TYPE=Debug this made it work but unsure exactly what is going on behind the scenes and whether it should stay here or not

Copy link
Member

Choose a reason for hiding this comment

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

-ldl is already linked for most things via the ${CMAKE_DL_LIBS}, included as part of the extra target dependencies. I'm guessing there's something not depending on extra that needs this? Whatever it is, it's better to do it as:

target_link_libraries(target INTERFACE ${CMAKE_DL_LIBS})

(where target is whatever the target is with the linking issue).

external/CMakeLists.txt Outdated Show resolved Hide resolved
src/cryptonote_config.h Outdated Show resolved Hide resolved
@darcys22
Copy link
Collaborator Author

This is now ready for review.

  • The macos tests are failing and are awaiting an upstream change which should resolve the issue. Disable std::filesystem on macOS when compiling C++17 but targetting <10.15 SRombauts/SQLiteCpp#335
  • There oxen_service_nodes_test_rollback test fails but is inconsistent, went over the codebase with Valgrind to see if I could find a memory leak/uninitialised variable which resulted in Initialise uptime proof info #1482 however I'm not sure if this was the root cause. Will have another go searching for this
  • Static builds are failing but I've exhausted my knowledge trying to get them to pass and need some help. CMake is hard
  • Discussion on optimal batching interval size to be scheduled

@darcys22 darcys22 marked this pull request as ready for review August 23, 2021 01:38
@darcys22 darcys22 requested a review from jagerman August 23, 2021 01:38
Copy link
Member

@jagerman jagerman left a comment

Choose a reason for hiding this comment

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

First pass, left many comments/suggestions, some tiny, some bigger.

More to come.

.drone.jsonnet Outdated Show resolved Hide resolved
.drone.jsonnet Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
@@ -86,6 +86,9 @@ else()
message(WARNING "PIE disabled: cmake 3.14+ is required for proper PIE linking support")
endif()

# TODO sean remove this
set(CMAKE_CXX_LINK_EXECUTABLE "${CMAKE_CXX_LINK_EXECUTABLE} -ldl")
Copy link
Member

Choose a reason for hiding this comment

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

-ldl is already linked for most things via the ${CMAKE_DL_LIBS}, included as part of the extra target dependencies. I'm guessing there's something not depending on extra that needs this? Whatever it is, it's better to do it as:

target_link_libraries(target INTERFACE ${CMAKE_DL_LIBS})

(where target is whatever the target is with the linking issue).

cmake/version.cpp.in Outdated Show resolved Hide resolved
external/CMakeLists.txt Outdated Show resolved Hide resolved
src/blockchain_db/sqlite/db_sqlite.cpp Outdated Show resolved Hide resolved
src/blockchain_db/sqlite/db_sqlite.cpp Outdated Show resolved Hide resolved
src/blockchain_db/sqlite/db_sqlite.cpp Outdated Show resolved Hide resolved
src/blockchain_db/sqlite/db_sqlite.cpp Outdated Show resolved Hide resolved
src/blockchain_db/sqlite/db_sqlite.cpp Outdated Show resolved Hide resolved
@darcys22 darcys22 force-pushed the batch-sn-payments branch 3 times, most recently from 8f3ae75 to 116624c Compare April 19, 2022 06:28
@darcys22 darcys22 force-pushed the batch-sn-payments branch from 8318c26 to 667ba98 Compare April 28, 2022 06:46
This updates the coinbase transactions to reward service nodes
periodically rather than every block. If you recieve a service node
reward this reward will be delayed x blocks, if you receive another
reward to the same wallet before those blocks have been completed it
will be added to your total and all will be paid out after those x
blocks has passed.

For example if our batching interval is 2 blocks:

Block 1 - Address A receives reward of 10 oxen - added to batch
Block 2 - Address A receives reward of 10 oxen - added to batch
Block 3 - Address A is paid out 20 oxen.

Batching accumulates a small reward for all nodes every block

The batching of service node rewards allows us to drip feed rewards
to service nodes. Rather than accruing each service node 16.5 oxen every
time they are pulse block leader we now reward every node the 16.5 /
num_service_nodes every block and pay each wallet the full amount that
has been accrued after a period of time (Likely 3.5 days).

To spread each payment evenly we now pay the rewards based on the
address of the recipient. This modulus of their address determines which
block the address should be paid and by setting the interval to our
service_node_batching interval we can guarantee they will be paid out
regularly and evenly distribute the payments for all wallets over this
@darcys22 darcys22 force-pushed the batch-sn-payments branch 2 times, most recently from e28a88a to 90fedba Compare May 1, 2022 22:50
@darcys22 darcys22 force-pushed the batch-sn-payments branch from 90fedba to 8bafaea Compare May 1, 2022 22:51
@jagerman jagerman merged commit 3ac7e55 into oxen-io:dev May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants