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.

Sign up for GitHub

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

Jump to bottom

Prepare Bisq for bitcoin-core v23 compatiblity #6219

Merged
merged 5 commits into from
Jul 5, 2022
Merged

Prepare Bisq for bitcoin-core v23 compatiblity #6219

merged 5 commits into from
Jul 5, 2022

Conversation

ghubstan
Copy link
Member

There seem to be at least two minor adjustments we have to make for bitcoin v23:

  • Add a CJDNS enum to DtoNetworkInfo.NetworkType, or json parsing will fail.
  • Remove -txindex=1 from seednode's bitcoin.conf.
    This change needs special attention from reviewers.

More detail about these two changes are in the commits.

The API test suites work with these changes, against both bitcoin-core v22 and v23, but have not been tested against bitcoin-core v21, 20, 19.

This PR needs more thorough testing with the desktop UI before merging.

Based on #6214, branch remove-bitcoind-txindex-param.

@ghubstan
Remove legacy bitcoind txindex param from apitest harness
0b66c2f 
It was put there thinking it would be needed by the test harness and suites,
which work fine without it, and it must be removed before API test harness
can run against bitcoin-core v23.

See bitcoin/bitcoin#22626 (comment)
@ghubstan
Adjust to CJDNS network support in bitcoin-core v23
7feecc9 
This change is backward compatible for bitcoin-core v22.
This change has not been tested against bitcoin-core v21, v20, v19.

See https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-23.0.md#p2p-and-network-changes
See https://github.com/bitcoin/bitcoin/blob/23.x/doc/cjdns.md
@ghubstan
Remove -txindex=1 from seednode's bitcoin.conf
3fe72b7 
This change needs thorough review before being merged.

See bitcoin-core Issue "Remove txindex migration code #22626":
  bitcoin/bitcoin#22626
And comment about change not having a release note:
  bitcoin/bitcoin#22626 (comment)

See src:   https://github.com/bitcoin/bitcoin/blob/v23.0/src/txdb.cpp#L35

For API test suites:
  This change is backward compatible for bitcoin-core v22.
  This change has not been tested against bitcoin-core v21, v20, v19.

Change needs to be thoroughly tested with desktop UI.
@ghubstan
Do not run test daemon in debug mode (by default)
9506386 
Thie test case is @disabled anyway, but run inside a suite instead,
where it was not running the test daemon in debug mode (anyway).
@ghubstan ghubstan mentioned this pull request May 26, 2022
Closed
@Emzy
Copy link
Contributor

Emzy commented Jun 13, 2022

Without this PR I see on bitcoin core version v23.0 this error:

[SeedNodeMain] ERROR b.c.d.n.f.FullNode: An error occurred: Error=bisq.core.dao.node.full.RpcException: com.fa
sterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `bisq.core.dao.node.full.rpc.dto.DtoNetworkI
nfo$NetworkType` from String "cjdns": not one of the values accepted for Enum class: [onion, ipv4, i2p, ipv6]
 at [Source: UNKNOWN; line: -1, column: -1] (through reference chain: bisq.core.dao.node.full.rpc.dto.DtoNetworkInfo["networks"]->
java.util.ArrayList[4]->bisq.core.dao.node.full.rpc.dto.DtoNetworkInfo$Network["name"])

With this PR no error is shown.

@ghubstan
Copy link
Member Author

ghubstan commented Jun 13, 2022 •
edited
Loading

Without this PR I see on bitcoin core version v23.0 this error:

[SeedNodeMain] ERROR b.c.d.n.f.FullNode: An error occurred: Error=bisq.core.dao.node.full.RpcException: com.fa
sterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `bisq.core.dao.node.full.rpc.dto.DtoNetworkI
nfo$NetworkType` from String "cjdns": not one of the values accepted for Enum class: [onion, ipv4, i2p, ipv6]
 at [Source: UNKNOWN; line: -1, column: -1] (through reference chain: bisq.core.dao.node.full.rpc.dto.DtoNetworkInfo["networks"]->
java.util.ArrayList[4]->bisq.core.dao.node.full.rpc.dto.DtoNetworkInfo$Network["name"])

With this PR no error is shown.

That issue is resolved by 7feecc9 in this PR #6219

@ghubstan ghubstan mentioned this pull request Jun 27, 2022
Closed
@Emzy
Copy link
Contributor

Emzy commented Jul 3, 2022

Why we are removing -txindex? It seems still be available on bitcoin core v23.0.
Maybe we don't need it anyway?

@ghubstan
Copy link
Member Author

ghubstan commented Jul 3, 2022

Why we are removing -txindex? It seems still be available on bitcoin core v23.0. Maybe we don't need it anyway?

They are migrating away from it, and are no release notes about it.
bitcoin/bitcoin#22626
https://github.com/bitcoin/bitcoin/blob/v23.0/src/txdb.cpp#L35

@Emzy
Copy link
Contributor

Emzy commented Jul 4, 2022

Why we are removing -txindex? It seems still be available on bitcoin core v23.0. Maybe we don't need it anyway?

They are migrating away from it, and are no release notes about it. bitcoin/bitcoin#22626 https://github.com/bitcoin/bitcoin/blob/v23.0/src/txdb.cpp#L35

As I understand it, there is a legacy txindex conversion removed. There is still the new txindex in place.

This seems not related to the CJDNS fix. Could you please separate the txindex change out so we can go forward with this PR.

@ghubstan
Revert "Remove -txindex=1 from seednode's bitcoin.conf"
3c5b04b 
This reverts commit 3fe72b7.

I made a mistake while testing this PR with API, and thought this txindex
option was breaking the regtest BSQ wallet.  The TX Index has been moved
to another database, and the -txindex option is still a valid option.

  $ bitcoind --version
  Bitcoin Core version v23.0.0

  $ bitcoind --help
    ...
    -txindex
       Maintain a full transaction index, used by the getrawtransaction rpc
       call (default: 0)
@ghubstan ghubstan changed the title [WIP] Prepare Bisq for bitcoin-core v23 compatiblity Prepare Bisq for bitcoin-core v23 compatiblity Jul 4, 2022
@ghubstan
Copy link
Member Author

ghubstan commented Jul 4, 2022

Why we are removing -txindex? It seems still be available on bitcoin core v23.0. Maybe we don't need it anyway?

They are migrating away from it, and are no release notes about it. bitcoin/bitcoin#22626 https://github.com/bitcoin/bitcoin/blob/v23.0/src/txdb.cpp#L35

As I understand it, there is a legacy txindex conversion removed. There is still the new txindex in place.

This seems not related to the CJDNS fix. Could you please separate the txindex change out so we can go forward with this PR.

It's not related to the not related to the CJDNS enum fix; it was related to making Bisq compatible with bitcoin-core 23. But oops you're right, the txindex flag can stay in the seed node conf file. I reverted commit 3fe72b7, and Bisq API regtest test suite passes for bitcoin-core v23.

@ghubstan ghubstan marked this pull request as ready for review July 4, 2022 15:34
@ripcurlx ripcurlx added this to the v1.9.5 milestone Jul 5, 2022
ripcurlx
ripcurlx approved these changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK

@ripcurlx ripcurlx merged commit bca997a into bisq-network:master Jul 5, 2022
@ghubstan ghubstan deleted the wip-prep-bisq-for-bitcoin-v23 branch July 5, 2022 11:11
@ghubstan ghubstan mentioned this pull request Jul 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ripcurlx ripcurlx ripcurlx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.9.5
Development

Successfully merging this pull request may close these issues.
3 participants
@ghubstan @Emzy @ripcurlx