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

Feature/functional tests updates segwit p2p ibd txrelay rpc getblockfrompeer p2p addconnections #124

Open
wants to merge 183 commits into
base: master
Choose a base branch
from

Conversation

gitcoindev
Copy link
Contributor

Description

Hi @wu-emma, @van-orton I am back -) I hope the project is still alive. Had issues with integration back and forth but finally had a breakthrough. This pull request initially was to tackle only segwit test but brings cherry-picked improvements from Bitcoin core integrated to Bitgesell:

  • speed up of test execution and enabling mocked time for functional tests
  • extended test/functional/p2p_ibd_txrelay.py
  • added rpc getblockfrompeer() and related test case test/functional/rpc_getblockfrompeer.py
  • removed feature_segwit and feature_nulldumy from default test executions as both test cases are related to segwit, which in Bitgesell is enabled from the genesis block
  • fixed p2p_add_connections.py test case

Just one functional test case rpc_packages.py is failing now, and wallet_importdescriptors.py is shaky, mostly passing when running standalone but fails with full test suite on importing the same descriptor with private key twice, both to be investigated.

slowriot and others added 30 commits August 20, 2021 00:20
…the over-zealous "bitcoin" -> "BGL" search-replace has resulted in these making no sense, and containing many broken links
… are broken links as they have been copied from bitcoincore.org and the equivalents do not exist at bitgesell.ca currently
…tcoin core, and turn off warning silencing for implicit fallthrough
It fails relocation related issue, and the linker advises to use -fpie
…ts_actualization

actualize functional tests
# Conflicts:
#	.appveyor.yml
#	build_msvc/README.md
#	contrib/gitian-build.py
#	contrib/gitian-descriptors/gitian-linux.yml
#	contrib/gitian-descriptors/gitian-osx.yml
#	contrib/gitian-descriptors/gitian-win.yml
#	contrib/install_db4.sh
#	doc/build-osx.md
#	doc/release-process.md
#	src/test/netbase_tests.cpp
#	src/util/system.cpp
#	test/README.md
…se58_fixes

# Conflicts:
#	contrib/testgen/base58.py
#	src/test/data/base58_encode_decode.json
…data.py

The string representation of a block header hash is simply the
hexlified byte-reversed double SHA256 hash of its serialization.
MarcoFalke and others added 17 commits April 18, 2023 15:36
We'll move the transaction relay data into Peer in subsequent commits,
but the inbound eviction logic needs to know if the peer is relaying
txs and if the peer has loaded a bloom filter.

This is currently redundant information with m_tx_relay->fRelayTxes,
but when m_tx_relay is moved into net_processing, then we'll need these
separate fields in CNode.
Also, remove cs_main guard from m_wtxid_relay_peers and make it atomic.
This should be fine since we don't need m_wtxid_relay_peers to be
synchronized with m_wtxid_relay exactly at all times.

After this change, RelayTransaction no longer requires cs_main.
This makes code that uses the helper less verbose.

Moreover, this makes net_processing C++20 compliant. Otherwise, it would
lead to a compile error (see below). C++20 disables aggregate
initialization when any constructor is declared. See
http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf

net_processing.cpp:1627:42: error: no matching constructor for initialization of 'CSerializedNetMsg'
            m_connman.PushMessage(pnode, CSerializedNetMsg{ser_cmpctblock.data, ser_cmpctblock.m_type});
                                         ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This makes code that uses the helper less verbose.

Moreover, this makes net_processing C++20 compliant. Otherwise, it would
lead to a compile error (see below). C++20 disables aggregate
initialization when any constructor is declared. See
http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf

net_processing.cpp:1627:42: error: no matching constructor for initialization of 'CSerializedNetMsg'
            m_connman.PushMessage(pnode, CSerializedNetMsg{ser_cmpctblock.data, ser_cmpctblock.m_type});
                                         ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
With this new method, a chain of transactions can be created. This
method is introduced to further simplify the mempool_package_limits.py
tests.
Moved `bulk_transaction` into MiniWallet class as `_bulk_tx` private
helper method to be used when the newly added `target_weight` option is
passed to `create_self_transfer*`
Fix the failing test case by using BGLTestFramework.
Refactoring only, no change in test behavior.
by generating 100 blocks for coinbase maturity instead of 427.

This speeds up the test and should help avoid timeout errors.
default.

Both tests are related to segwit which is by default enabled in the
Bitgesell genesis block, therefore invalid and just consumes test execution time.
Also, the segwit test case proved to be shaky on Bitcoin and recently partially disabled,
the changes were cherr-picked as well for consistency.

bitcoin/bitcoin#24421 (comment)
bitcoin/bitcoin#24421 (comment)
@wu-emma
Copy link
Collaborator

wu-emma commented Jun 19, 2023

@gitcoindev thanks for participation!
Hi @janus could you please take a look at this PR?
ping @van-orton

@janus
Copy link
Collaborator

janus commented Jun 19, 2023

@gitcoindev thanks for participation! Hi @janus could you please take a look at this PR? ping @van-orton

Alright.

@janus
Copy link
Collaborator

janus commented Jun 20, 2023

@gitcoindev
Did you try with clang? Something like below
./configure CXX=clang++ CC=clang --with-incompatible-bdb

@janus
Copy link
Collaborator

janus commented Jun 20, 2023

@gitcoindev
I noticed that you committed some pretty new commits from bitcoin. Is it possible to work with this branch, https://github.com/BitgesellOfficial/bitgesell/tree/bitcoinsync2022? It is updated to 2022.

@gitcoindev
Copy link
Contributor Author

gitcoindev commented Jun 20, 2023 via email

@janus
Copy link
Collaborator

janus commented Jun 21, 2023

Hi @janus thank you for checking, I can try to rebase to https://github.com/BitgesellOfficial/bitgesell/tree/bitcoinsync2022 , could you please provide the exact commands that you use to build your 2022 branch so that I have the same setup as yours? wt., 20 cze 2023 o 11:20 Emeka @.> napisał(a):

@gitcoindev https://github.com/gitcoindev I noticed that you committed some pretty new commits from bitcoin. Is it possible to work with this branch, https://github.com/BitgesellOfficial/bitgesell/tree/bitcoinsync2022? It is updated to 2022. — Reply to this email directly, view it on GitHub <#124 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVFGLNPLTNMMYVJWILYZSSDXMFTO5ANCNFSM6AAAAAAZJA5TAM . You are receiving this because you were mentioned.Message ID: @.
>

Please I would prefer if you work on branch bitcoinsync2022. It has around 4% unit tests that are failing and about 20% integration tests that are still failing. Don't use commits that are younger than 31st December, 2022.

It should be able to work with any configuration setting.

@gitcoindev
Copy link
Contributor Author

This will take a while as I am having compilation issues and need to figure out the root cause. I will reopen when it's ready.

@gitcoindev gitcoindev closed this Jun 22, 2023
@gitcoindev
Copy link
Contributor Author

gitcoindev commented Jun 26, 2023

Hi @janus I was able to build as well this PR with clang - just had to use newer clang 14 and not the default clang for Ubuntu 20.04 using the following steps.

$ wget https://apt.llvm.org/llvm.sh
$ chmod +x ./llvm.sh 
$ sudo ./llvm.sh 14
$ ./configure --with-incompatible-bdb --enable-suppress-external-warnings CC=clang-14 CCX=clang++-14
$ make clean
$ make -j16

Feel free to merge this one or reject the PR to master. I will also try to cherry pick to bitcoin2022 branch with commits younger than 31st December, 2022 (might take me until Friday).

@gitcoindev gitcoindev reopened this Jun 26, 2023
@naftalimurgor
Copy link
Contributor

Hey @gitcoindev

This is great, it would be great to have this work for both Clang 14+ and GCC. Also, the bitcoinsync2022 branch has a lot of commits that are synced from Bitcoin Core

@janus
Copy link
Collaborator

janus commented Jun 27, 2023

Hi @janus I was able to build as well this PR with clang - just had to use newer clang 14 and not the default clang for Ubuntu 20.04 using the following steps.

$ wget https://apt.llvm.org/llvm.sh
$ chmod +x ./llvm.sh 
$ sudo ./llvm.sh 14
$ ./configure --with-incompatible-bdb --enable-suppress-external-warnings CC=clang-14 CCX=clang++-14
$ make clean
$ make -j16

Feel free to merge this one or reject the PR to master. I will also try to cherry pick to bitcoin2022 branch with commits younger than 31st December, 2022 (might take me until Friday).

Great!
We are in the process of updating master branch, I would prefer if you continue working on bitcoinsync2022 failing tests.
However, we may cherry-pick from that PR in the future.

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.