Skip to content

Backported chainwork fix#45

Merged
bisqadmin merged 10 commits intobisq-network:mainfrom
HenrikJannsen:backported_chainwork_fix
Dec 23, 2025
Merged

Backported chainwork fix#45
bisqadmin merged 10 commits intobisq-network:mainfrom
HenrikJannsen:backported_chainwork_fix

Conversation

@HenrikJannsen
Copy link
Copy Markdown
Collaborator

@HenrikJannsen HenrikJannsen commented Dec 23, 2025

This cherry-picked the commits suggested in:

bitcoinj#3975 (comment)

but excluding the checkpoint related commits as suggested by @schildbach in #45 (comment) to reduce risks and as those changes are not strictly required for the hotfix.

Additional commits for missing methods and updates in build files and CI config have been applied as well.

msgilligan and others added 2 commits June 24, 2024 00:28
This is a fix for Issue bitcoinj#3410 that will allow main net blocks above
849,137 to be processed. As of block 849,138 we can no longer fit
total chainwork in a 12-byte *signed* field. This fix "kicks the can
down the road" by making the field 12-bytes *unsigned*.

We should open a new issue to address the long term need for bigger
values. Note that converting the field to 12-byte unsigned precludes
us from using the most-significant bit as a flag for a new format, but
we should be able to pick some arbitrary value, say 0xA0 as a version
flag and declare that values less than 0xA0 are "unversioned".

This is BACKPORT to the 0.16 branch. In the master/0.17 branch
the bigIntegerToBytes method has been moved to base.internal.BytUtils,
so this commit differs in that one respect.
The old format will soon run out of bytes for the chain work value.
@HenrikJannsen HenrikJannsen force-pushed the backported_chainwork_fix branch 12 times, most recently from 898691f to 3cc5f29 Compare December 23, 2025 10:00
@HenrikJannsen
Copy link
Copy Markdown
Collaborator Author

Windows CI build fails at

assertTrue(watch.elapsed(TimeUnit.MILLISECONDS) >= timeout); // should not disconnect before timeout
. I guess this is not related to the code changes.

HenrikJannsen added a commit to HenrikJannsen/bisq that referenced this pull request Dec 23, 2025
… backport of the chainwork bugfix.

See bisq-network/bitcoinj#45

Signed-off-by: HenrikJannsen <boilingfrog@gmx.com>
@schildbach
Copy link
Copy Markdown

schildbach commented Dec 23, 2025

Not sure if it's worth the effort, but in order to lower the risk of regressions you could leave out the BuildCheckpoint-related changes (including the picocli dependency) for now. Reasons:

  • It will take a bit until a new checkpoint is due and then there is the "safety margin" (default: 7 days) that comes on top of it.
  • Even if the checkpoints would not be updated any more, the effect on user experience would be gradual (minimal at first, only getting more severe after a couple of months have passed without up-to-date checkpoints).
  • If you want to, you can always generate an up-to-date checkpoints file using bitcoinj 0.17 – no need to use your fork for that.

If you decide you don't need more recent checkpoints for the next couple of weeks at all (again, the impact will be minimal at first), then you could also exclude the changes related to CheckPointmanager from this PR.

In other words: For a hotfix, only the changes related to SPVBlockStore are truly relevant. Checkpoints could come next year when the responsible developer is available again.

In any case, I'd strongly recommend developing a strategy to update bitcoinj at least to version 0.16.x which we will probably support with critical updates for some time.

@HenrikJannsen
Copy link
Copy Markdown
Collaborator Author

Not sure if it's worth the effort, but in order to lower the risk of regressions you could leave out the BuildCheckpoint-related changes (including the picocli dependency) for now. Reasons:

* It will take a bit until a new checkpoint is due and then there is the "safety margin" (default: 7 days) that comes on top of it.

* Even if the checkpoints would not be updated any more, the effect on user experience would be gradual (minimal at first, only getting more severe after a couple of months have passed without up-to-date checkpoints).

* If you want to, you can always generate an up-to-date checkpoints file using bitcoinj 0.17 – no need to use your fork for that.

If you decide you don't need more recent checkpoints for the next couple of weeks at all (again, the impact will be minimal at first), then you could also exclude the changes related to CheckPointmanager from this PR.

In other words: For a hotfix, only the changes related to SPVBlockStore are truly relevant. Checkpoints could come next year when the responsible developer is available again.

In any case, I'd strongly recommend developing a strategy to update bitcoinj at least to version 0.16.x which we will probably support with critical updates for some time.

Thanks! OK, I will remove those commits. I did not plan to provide checkpoint updates in that release, only the minimal fixes, but for sure lowers the risk in such a quick hotfix.

Yes we are aware that its very bad that we are such far behind the upstream version, but as Musig on Bisq 2 will use BDK and we put all dev efforts into that, we tried to limit efforts on Bisq 1 - which went obviously too far by not following up better with this issue...

@schildbach
Copy link
Copy Markdown

On another note, I'm confused about the presence of a backport of our "kicking the can down the road" hotfix on this PR. I thought Bisq has already hotfixed this when the issue came up in June 2024?

Are you sure this PR is against the correct branch that is actually used in Bisq?

@HenrikJannsen
Copy link
Copy Markdown
Collaborator Author

@schildbach Do you think the failed test (#45 (comment)) is critical? It seems to me a CI environment issue with timing. I tried a bit with newer versions but that caused then other issues.

schildbach and others added 8 commits December 23, 2025 19:09
This gets rid of several bytes to/from string conversions.
This is meant to make sure the buffer position is always right.
Existing V1 files are automatically migrated to V2 format.

Includes a test for migration from V1 to V2 format. This requires
`getRingCursor()` to be changed from private to package-private.
- Use protobuf-java:3.18.0 instead of protobuf-java:3.6.1
- Use mavenCentral instead of jcenter
@HenrikJannsen
Copy link
Copy Markdown
Collaborator Author

On another note, I'm confused about the presence of a backport of our "kicking the can down the road" hotfix on this PR. I thought Bisq has already hotfixed this when the issue came up in June 2024?

Are you sure this PR is against the correct branch that is actually used in Bisq?

Yes, the latest Bisq release used commit b005953e5eec339a82daf4866f85518091f6b9f6 of our BitcoinJ branch.

@HenrikJannsen HenrikJannsen force-pushed the backported_chainwork_fix branch from 3cc5f29 to 7dc0851 Compare December 23, 2025 12:13
@schildbach
Copy link
Copy Markdown

@schildbach Do you think the failed test (#45 (comment)) is critical? It seems to me a CI environment issue with timing. I tried a bit with newer versions but that caused then other issues.

I don't think it's caused by a regression introduced by this PR. More likely it's a fluke of these PeerGroupTest tests. If you re-run the failed test using the GitHub UI, is the failure persistent?

@HenrikJannsen
Copy link
Copy Markdown
Collaborator Author

@schildbach Do you think the failed test (#45 (comment)) is critical? It seems to me a CI environment issue with timing. I tried a bit with newer versions but that caused then other issues.

I don't think it's caused by a regression introduced by this PR. More likely it's a fluke of these PeerGroupTest tests. If you re-run the failed test using the GitHub UI, is the failure persistent?

Before it was failing on Windows with Java 15 now with Java 11. So its not persistent, but seems Windows is mostly affected.

HenrikJannsen added a commit to HenrikJannsen/bisq that referenced this pull request Dec 23, 2025
@bisqadmin
Copy link
Copy Markdown

utACK

Copy link
Copy Markdown

@hiciefte hiciefte left a comment

Choose a reason for hiding this comment

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

utACK

@bisqadmin bisqadmin merged commit 52bfceb into bisq-network:main Dec 23, 2025
11 of 12 checks passed
HenrikJannsen added a commit to HenrikJannsen/bisq that referenced this pull request Dec 23, 2025
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