Skip to content

Re-factor/re-name blockdata#1296

Closed
tcharding wants to merge 3 commits intorust-bitcoin:masterfrom
tcharding:09-20-re-org-blockdata
Closed

Re-factor/re-name blockdata#1296
tcharding wants to merge 3 commits intorust-bitcoin:masterfrom
tcharding:09-20-re-org-blockdata

Conversation

@tcharding
Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

@Kixunil
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@tcharding tcharding force-pushed the 09-20-re-org-blockdata branch from d5e69d8 to d06d7cc Compare September 21, 2022 00:45
@tcharding
Copy link
Copy Markdown
Member Author

Fixed paths in fuzz code.

@tcharding
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@tcharding tcharding force-pushed the 09-20-re-org-blockdata branch from d06d7cc to 6cd2798 Compare September 21, 2022 23:21
@Kixunil
Copy link
Copy Markdown
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 tcharding force-pushed the 09-20-re-org-blockdata branch from 6cd2798 to 302f386 Compare September 30, 2022 02:14
@tcharding
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

tcharding commented Jan 29, 2024

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

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jan 29, 2024

keep getting tied in knots.

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

@tcharding
Copy link
Copy Markdown
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.

4 participants