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

Re-factor/re-name blockdata #1296

Closed

Conversation

tcharding
Copy link
Member

This is a re-do of the first three patches in #525. The git log for each commit is a bit light on why we should be doing this (because I'm more or less blindly following #525 after about 30 seconds of "hmm seems reasonable").

@tcharding
Copy link
Member Author

Woops fuzzing, will re-spin tomorrow if I get a concept ACK.

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 20, 2022

Why renaming? I think blockdata was fine.

@Kixunil Kixunil added crate smashing Issues and PRs splitting out parts of the main crate to smaller crates and removed crate smashing Issues and PRs splitting out parts of the main crate to smaller crates labels Sep 20, 2022
@dpc
Copy link
Contributor

dpc commented Sep 20, 2022

Why renaming? I think blockdata was fine.

I was not participating in any discussions about it, but to me it always seemed superfluous, and block data seems and its decoding and encoding seems like a centerpiece of the whole crate. And generally there module namespace seems to have more depth then necessary.

@apoelstra
Copy link
Member

The goal here is to move this stuff into its own crate, which would contain this data as well as compact blocks (bip 158), maybe addresses and hashes, etc., which is not all "block data".

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 20, 2022

In my experience Transaction and everything it contains is used very often, almost always. OTOH use of things like network or bip* is very project-dependent.

@apoelstra
Copy link
Member

@Kixunil right, so it would be nice if Transaction etc were in its own crate without any of those other things

@tcharding
Copy link
Member Author

Fixed paths in fuzz code.

@tcharding
Copy link
Member Author

If primitives is to become a crate would Amount go into it or do we want it separate in bitcoin-units still?

@apoelstra
Copy link
Member

IMHO, if Amount would force a dependency of primitives on -units, then it should be in primitives and not units.

But if the dependency would exist anyway, which I suspect would be the case, then Amounts more properly belongs in -units.

caveat: I am just spitballing here and will probably forget I said this the next time we have a problem that suggests the opposite thing would be wise :)

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 21, 2022

Is primitives supposed to mean blockdata? If yes, it's not a good name because I already don't know what goes into them.

if the dependency would exist anyway, which I suspect would be the case

Indeed, methods like target(), work(), witness_size() will return units. We could make them optional but that seems too much. This touches the question of should TxAmount be a separate type? If yes, then I'd be more open to having units optional but still not convinced.

@apoelstra
Copy link
Member

All the bitcoin primitives go into bitcoin-primitives. To a first approximation, everything defined in src/primitives/*.h in Bitcoin Core. blockdata is weird and rust-bitcoin-specific and as I've mentioned, I don't think it's properly descriptive.

I think conceptually bitcoin-primitives should depend on bitcoin-units. I could also get behind -primitives and -units being merged.

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 21, 2022

Defining things by in which file Core has them doesn't sound great.

Re-name the `blockdata` module to `primitives`.

Refactor only, no logic changes.
Move the `script` module out of `primitives` and into the crate root.
Move `opcodes` to the newly created `script` module.

Refactor only, no logic changes.
Inline the code out of `primitives/constants.rs` into
`primitives/mod.rs`.

Refactor only, no logic changes.
@tcharding
Copy link
Member Author

I've rebased but I don't know whats the decision on this one, perhaps we should leave this until we discussed our next moves. Converting to draft.

@tcharding tcharding marked this pull request as draft September 30, 2022 02:17
@tcharding tcharding added the crate smashing Issues and PRs splitting out parts of the main crate to smaller crates label Dec 20, 2022
@Kixunil
Copy link
Collaborator

Kixunil commented Jan 27, 2024

I'm thinking I will try to pull Wight and FeeRate into units once the #2408 is merged, then move around decoder impls from #2184 and try to pull primitives out.

@tcharding
Copy link
Member Author

tcharding commented Jan 29, 2024

Sweet, I've had a couple of goes and keep getting tied in knots.

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 29, 2024

keep getting tied in knots.

Oh, hell, this is much worse than I thought...

@tcharding
Copy link
Member Author

Consumed by #2756

@tcharding tcharding closed this May 20, 2024
@tcharding tcharding deleted the 09-20-re-org-blockdata branch May 22, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate smashing Issues and PRs splitting out parts of the main crate to smaller crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants