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

Core Improvements and Bugfixes #89

Merged
merged 26 commits into from
May 1, 2022

Conversation

SmartArray
Copy link

@SmartArray SmartArray commented Apr 7, 2022

Core Improvements and Bugfixes

This PR fixes several bugs that were detected by fixing the functional tests. Those fixes include for instance

  • Dandelion bugfixes (mempool bugs)
  • RPC bugfixes (generatetoaddress, difficulties)
  • Fee improvements
  • Introduction of easypow parameter for RegTest
  • Several refactorings
  • Fixed testnet issues
  • Bumped protocol versions
  • Proper handling of WTXID protocol command

Julian Jäger and others added 25 commits April 6, 2022 13:49
This will be needed to postpone difficulty adjustment modifications to a block in the future throughout the functional tests
Copy link

@JaredTate JaredTate left a comment

Choose a reason for hiding this comment

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

ACK. Awesome work @SmartArray! This seems to finally have eliminated the crash bug with dandelion. Going commit by commit it all looks good. The client compiles, runs, and works as expected. One question I had was the commit about DEFAULT_MIN_RELAY_TX_FEE = 100000. Thats 1/100 of a DGB. Should we make it 1/10?

Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

ACK

Excellent work on this @SmartArray ! It is absolutely refreshing to be so much closer to having the functional test suite fully operational. I'm looking forward to review #90 !

@barrystyle
Copy link

So it was this legacy code that was causing a transaction to effectively be removed twice?
9be9458

However this identical code appears in the 0.17 branch as well:
https://github.com/DigiByte-Core/digibyte/blob/develop/src/txmempool.cpp#L526-L530

@barrystyle
Copy link

barrystyle commented Apr 25, 2022

image

Receive this on feature/8.22.0 with pull request #89 applied (note that digibyted itself is fine).

@barrystyle
Copy link

image

Receive this on feature/8.22.0 with pull request #89 applied (note that digibyted itself is fine).

If you remove the additional primitives/block.cpp added to the Makefile.am:

diff --git a/src/Makefile.am b/src/Makefile.am
index 9ba20eadcc..42f90ee164 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -668,6 +668,7 @@ libdigibyte_cli_a_SOURCES = \
   compat/stdin.h \
   compat/stdin.cpp \
   rpc/client.cpp \
+  primitives/block.cpp \
   $(DIGIBYTE_CORE_H)

and instead make this change to Makefile.am:

diff --git a/src/Makefile.am b/src/Makefile.am
index 9ba20eadc..40573435c 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -724,6 +724,8 @@ endif
 digibyte_cli_LDADD = \
   $(LIBDIGIBYTE_CLI) \
   $(LIBUNIVALUE) \
+  $(LIBDIGIBYTE_COMMON) \
+  $(LIBDIGIBYTE_CONSENSUS) \
   $(LIBDIGIBYTE_UTIL) \
   $(LIBDIGIBYTE_CRYPTO)

It will compile correctly with no symbol errors.

@barrystyle
Copy link

Upon running unit tests:

root@loderunner:~/digibyte/src/test# ./test_digibyte 
Running 470 test cases...
test_digibyte: test/util/setup_common.cpp:226: TestChain100Setup::TestChain100Setup(): Assertion `m_node.chainman->ActiveChain().Tip()->GetBlockHash().ToString() == "1b14e04a06ce040bf88b9de8f4c2c4bb38c125acefcbdc2fc841d18bb02cdff6"' failed.
unknown location(0): fatal error: in "blockfilter_index_tests/blockfilter_index_initial_sync": signal: SIGABRT (application abort requested)
test/blockfilter_index_tests.cpp(105): last checkpoint: "blockfilter_index_initial_sync" fixture ctor
test_digibyte: util/system.cpp:654: void ArgsManager::AddArg(const string&, const string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.

@SmartArray
Copy link
Author

Thank you @barrystyle for testing this.

Yes, I think this is the correct way to deal with the linking error.

There were several issues regarding the mempool. And yes, the code you highlighted was absolutely in there before. That would explain why reorgs caused the client to misbehaving on occasion.

Again, thank you for checking this 🙂

I will apply and check the fix soon!

@SmartArray
Copy link
Author

Upon running unit tests:


root@loderunner:~/digibyte/src/test# ./test_digibyte 

Running 470 test cases...

test_digibyte: test/util/setup_common.cpp:226: TestChain100Setup::TestChain100Setup(): Assertion `m_node.chainman->ActiveChain().Tip()->GetBlockHash().ToString() == "1b14e04a06ce040bf88b9de8f4c2c4bb38c125acefcbdc2fc841d18bb02cdff6"' failed.

unknown location(0): fatal error: in "blockfilter_index_tests/blockfilter_index_initial_sync": signal: SIGABRT (application abort requested)

test/blockfilter_index_tests.cpp(105): last checkpoint: "blockfilter_index_initial_sync" fixture ctor

test_digibyte: util/system.cpp:654: void ArgsManager::AddArg(const string&, const string&, unsigned int, const OptionsCategory&): Assertion `ret.second' failed.

Found that too. This is because I had to change some network params

Previous change to Makefile.am caused Linking issue on Debian based systems.
@SmartArray
Copy link
Author

Hey guys, please try to ./autogen.sh, ./configure and make once more! I have integrated the change as proposed by @barrystyle

Copy link

@barrystyle barrystyle left a comment

Choose a reason for hiding this comment

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

ACK Happy with these changes

@ycagel
Copy link
Member

ycagel commented May 1, 2022

@barrystyle Thanks for the review. Are you able to do PR 90?

@gto90
Copy link
Member

gto90 commented May 1, 2022

Very strong work @SmartArray ! Thank you for all your hard effort here.

@gto90 gto90 merged commit 283a7b4 into DigiByte-Core:feature/8.22.0 May 1, 2022
@barrystyle
Copy link

@barrystyle Thanks for the review. Are you able to do PR 90?

can do.

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.

None yet

5 participants