Skip to content

BlockchainLMDB: do not assume alignment for alt block entries#10046

Merged
tobtoht merged 1 commit intomonero-project:masterfrom
jeffro256:lmdb_align_alt_block_entries
Aug 30, 2025
Merged

BlockchainLMDB: do not assume alignment for alt block entries#10046
tobtoht merged 1 commit intomonero-project:masterfrom
jeffro256:lmdb_align_alt_block_entries

Conversation

@jeffro256
Copy link
Copy Markdown
Contributor

Issue with alignment described in #10045. A full database migration shouldn't be required since alt blocks are flushed on startup.

Copy link
Copy Markdown
Contributor

@vtnerd vtnerd left a comment

Choose a reason for hiding this comment

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

This should do as advertised, but not certain how this is going to be ported to 0.18 with this C++17 code.

@hyc
Copy link
Copy Markdown
Collaborator

hyc commented Aug 19, 2025

Sorry, I'm not seeing how this helps anything. It looks like all you're doing is aligning the memory buffer that you're passing to LMDB. That buffer's alignment is irrelevant, since it has nothing to do with where in the mmap the value will actually get written.

As I said in #10045, if you want to preserve alignment, you must pad the size of the block blob to the proper alignment, before writing it to LMDB.

@jeffro256
Copy link
Copy Markdown
Contributor Author

Ah okay, then I misunderstood your original answer, will rework now.

@jeffro256 jeffro256 marked this pull request as draft August 20, 2025 16:24
@jeffro256 jeffro256 force-pushed the lmdb_align_alt_block_entries branch from 25a2488 to a5b2ad0 Compare August 21, 2025 04:28
@jeffro256 jeffro256 changed the title BlockchainLMDB: align new alt block entries in the DB BlockchainLMDB: do not assume alignment for alt block entries Aug 21, 2025
@jeffro256 jeffro256 marked this pull request as ready for review August 21, 2025 04:29
@jeffro256
Copy link
Copy Markdown
Contributor Author

I implemented padding locally, but it was a lot more complicated than just not assuming alignment, so that's what I did in the latest commit.

@jeffro256
Copy link
Copy Markdown
Contributor Author

Msys compile error is not relevant to this PR, see #10036.

Copy link
Copy Markdown
Collaborator

@hyc hyc left a comment

Choose a reason for hiding this comment

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

Simple enough.

@tobtoht tobtoht merged commit 0229f6f into monero-project:master Aug 30, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants