Unsubtree Univalue #25369

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:unsubtree_univalue changing 33 files +32 −1345
  1. fanquake commented at 11:48 AM on June 14, 2022: member

    At this point, maintaining Univalue as a subtree doesn’t serve much purpose, other than being an inconvenience for making changes to the code (along with polluting our repo with a number of files we don’t use). Our Univalue fork currently deviates from the upstream API, and for some time has been marked as not-maintained for use by other projects (I'm not aware of any that use it). The upstream Univalue is not maintained, and has not been for some time. There are no new releases, bugs remain unfixed, and PR's we've upstreamed, https://github.com/jgarzik/univalue/pulls, are not being commented on/merged.

    Another substantial benefit of no-longer maintaining a subtree is removing the rather awkward work-flow currently required to make changes to the Univalue code, particularly breaking changes / introducing new features, e.g. https://github.com/bitcoin-core/univalue-subtree/pull/27. We need to dance around and merge changes to our fork, with a flag, then pull them down here, then switch to using the new code, then go back to our Univalue repo, and remove the old code / flag, then pull the repo down here again, and remove our usage of the flag. Quite the overcomplicated mess.

    With this PR I'm proposing we stop treating Univalue like a subtree, or upstream project/fork, and going forward, treat it as part of this codebase, which we can refactor directly (with pulls to this repo. Ideally, after this is merged, our univalue subtree repo could be marked as "archived". In this repo, I think there is a good chance that the Univalue code will ultimately be refactored away into "modern" C++, i.e using std::variant (at least one person has played around with doing this).

    Univalue history:

    Guix Build (x86_64):

    06748985a9a386457d10a411b5afe1d59536e5653ec9c5bc8ac8410cd715d073  guix-build-d873ff96e51a/output/aarch64-linux-gnu/SHA256SUMS.part
57d81891f6d4ae417dd3bcbfc90839600e103da9c7d7b09dbebb82f0119241f3  guix-build-d873ff96e51a/output/aarch64-linux-gnu/bitcoin-d873ff96e51a-aarch64-linux-gnu-debug.tar.gz
7bb70d3b67253f5e8e5af8158bbf1b4b3e25e782f951d3defb7976534ae67d62  guix-build-d873ff96e51a/output/aarch64-linux-gnu/bitcoin-d873ff96e51a-aarch64-linux-gnu.tar.gz
b1acb90877d6e3b8d4bd2d57103889e0474263e4153f302eba8cb304fd1aecd7  guix-build-d873ff96e51a/output/arm-linux-gnueabihf/SHA256SUMS.part
91f9f65aebc131522cae5b523359c62e402a2c929670e1cca19d6a2760d29e04  guix-build-d873ff96e51a/output/arm-linux-gnueabihf/bitcoin-d873ff96e51a-arm-linux-gnueabihf-debug.tar.gz
1fc3ed39bfc95592503b8dd11f468240deca4fb757f9adb08a0f07f5c0690837  guix-build-d873ff96e51a/output/arm-linux-gnueabihf/bitcoin-d873ff96e51a-arm-linux-gnueabihf.tar.gz
a5cf5bd0ee0de92fb03f6bca91cfa6667ed77885112e71dd92a82bbd8670141e  guix-build-d873ff96e51a/output/arm64-apple-darwin/SHA256SUMS.part
f6715399cebb5ac0a09f190fe805146c13d1e8eba57401541d0628da3badc588  guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin-unsigned.dmg
07cf82cab4e459ed4e862fc3a2903e49ac750adc6b6fe0534ec165f00e666230  guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin-unsigned.tar.gz
81bc076aa415183109e2848fa3cc0265b34f6af3e75b76bcbc6cff524db76a0f  guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin.tar.gz
8751b05a3395d668e31217c92cbce9c131aa3566b3784a7e3544adf34fc89fe8  guix-build-d873ff96e51a/output/dist-archive/bitcoin-d873ff96e51a.tar.gz
526b7780a16a3de3c6006606d3d7a8c2ca565ef28669e2f6f303349a252e4977  guix-build-d873ff96e51a/output/powerpc64-linux-gnu/SHA256SUMS.part
ff917a50d2b20d41a5954e1ba1e8fb39498a9c8867828483af3f501573148ede  guix-build-d873ff96e51a/output/powerpc64-linux-gnu/bitcoin-d873ff96e51a-powerpc64-linux-gnu-debug.tar.gz
0311455c821ad392013fc3999a2b2d027fdb5c28e7eb6c3fea9cec29f3730d2d  guix-build-d873ff96e51a/output/powerpc64-linux-gnu/bitcoin-d873ff96e51a-powerpc64-linux-gnu.tar.gz
983c2553990eb7cebb26e1a0a3e5a9308259dea60d0b64ab6782892d02a7abc1  guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/SHA256SUMS.part
aba604827d969348671ec3f36dbf37469292715d3f756a7f44a0a5243dbe02f3  guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/bitcoin-d873ff96e51a-powerpc64le-linux-gnu-debug.tar.gz
e450bd82020d5086f3bb0a23181263315cc05eaf6e5809d0a2115bff4e7ddb2e  guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/bitcoin-d873ff96e51a-powerpc64le-linux-gnu.tar.gz
476e8e2c80498b241af154abd9112bd2767110c0d6d7e9fa11761de716cb760f  guix-build-d873ff96e51a/output/riscv64-linux-gnu/SHA256SUMS.part
a76435b3492efcd9af47ad652170605fad50691fd5aff2b46bce0bd08014879e  guix-build-d873ff96e51a/output/riscv64-linux-gnu/bitcoin-d873ff96e51a-riscv64-linux-gnu-debug.tar.gz
83985d409cd90bf7120cf7902ee442595d28a1469b7c600b666ef901981e5190  guix-build-d873ff96e51a/output/riscv64-linux-gnu/bitcoin-d873ff96e51a-riscv64-linux-gnu.tar.gz
61c89850244ddf5813ff80c242eff89925d30bccadfa5cb63e968c3af49eb964  guix-build-d873ff96e51a/output/x86_64-apple-darwin/SHA256SUMS.part
cd219fab8918b061a342357d298aca0c044feb34c6d50a7851d5d3bf18cec267  guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin-unsigned.dmg
1170d3fdb199fbfca2c20b2a77cc81a6fe24b7e4973543a4461e887f14ac68e9  guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin-unsigned.tar.gz
71e93297ed8c581a7ed32a6948ef7b1ea2e7c43cb054181de3b5f604f7a2c28b  guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin.tar.gz
fc8b7b670de9d175775e73df47dc855581c873a9be4adf1d81a4dbb2831d5348  guix-build-d873ff96e51a/output/x86_64-linux-gnu/SHA256SUMS.part
5703b02c2647f9997aa5ca12514d6a54b1eb2e29046223ca062383326b95894f  guix-build-d873ff96e51a/output/x86_64-linux-gnu/bitcoin-d873ff96e51a-x86_64-linux-gnu-debug.tar.gz
bab4b932b83476cf6fc2e0b5bf0d2203287f7fd0d1a968e325f2edd5b1d8415b  guix-build-d873ff96e51a/output/x86_64-linux-gnu/bitcoin-d873ff96e51a-x86_64-linux-gnu.tar.gz
5d180b0415fa8e825d46928c168cb1ae6e27016841b2ff8e190bf13879a5545c  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/SHA256SUMS.part
d469695a32f6414b25fef7b5fdfda4d854071450ba25148a1dce468114fa9057  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-debug.zip
2e7d4e533a5998863c115c586c61b75b4039cd329e12ed24cff78b7f16b6ea57  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-setup-unsigned.exe
3dabbd627b532beef57c3d4b5bd30c93c5ea74c492918484cf24685aca8d7bc4  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-unsigned.tar.gz
3a40660fba08f7632efd1f73c198f8298db33eab6ef5eaca88b997d95fc31f29  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64.zip

    Guix Build (arm64):

    0e764679199358fc321dcfcb58c6302e6518f55b3fd27bdd47f2da2a826ba16a  guix-build-d873ff96e51a/output/arm-linux-gnueabihf/SHA256SUMS.part
5955d28e6d56e5a3297dab723b8478f1b0bb7f5b86476c581339122f34cc7f14  guix-build-d873ff96e51a/output/arm-linux-gnueabihf/bitcoin-d873ff96e51a-arm-linux-gnueabihf-debug.tar.gz
49c68bc0066f709be68f1e5731425d51fb3cb8062a24aa9fa599987165759cad  guix-build-d873ff96e51a/output/arm-linux-gnueabihf/bitcoin-d873ff96e51a-arm-linux-gnueabihf.tar.gz
ca678d4eb27c9fa3c527211c0ccb145322a15f327545b5c82f1d1b8d3c310e5a  guix-build-d873ff96e51a/output/arm64-apple-darwin/SHA256SUMS.part
38366d7fbd769b426f1097e966abe39f01a7ce743f6af1cd0f228b1801d3c87f  guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin-unsigned.dmg
0c05dc9c17f5d8237b3e003c2e4c715455c3868bd4cd014e2a15ceb152b27b9c  guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin-unsigned.tar.gz
32676e1f9f07f3f77143f8b6038c943da6ba93b081232ec52c2ff940f9f7cc88  guix-build-d873ff96e51a/output/arm64-apple-darwin/bitcoin-d873ff96e51a-arm64-apple-darwin.tar.gz
8751b05a3395d668e31217c92cbce9c131aa3566b3784a7e3544adf34fc89fe8  guix-build-d873ff96e51a/output/dist-archive/bitcoin-d873ff96e51a.tar.gz
bdae66515060cab0b362784f0b2019b77da0435f1732d3c91fabcfb5e8c675f6  guix-build-d873ff96e51a/output/powerpc64-linux-gnu/SHA256SUMS.part
8d837391310b4cdec2296a6e78a9f9b3ea2b3da7870881a5cedf86a3429c08c6  guix-build-d873ff96e51a/output/powerpc64-linux-gnu/bitcoin-d873ff96e51a-powerpc64-linux-gnu-debug.tar.gz
efe825d6f36338bd4c0b427901b72d666f819858fb241a4211f03bbb738f6961  guix-build-d873ff96e51a/output/powerpc64-linux-gnu/bitcoin-d873ff96e51a-powerpc64-linux-gnu.tar.gz
7494cf8c5f384ca3205b3ed44dd4c0edebcb9e0a6bf9c8e649fc6d99cc5a10b2  guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/SHA256SUMS.part
8ceeb21d7fce9e164dbb47b35d0551b59819075fc44dcea39603132340f80c41  guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/bitcoin-d873ff96e51a-powerpc64le-linux-gnu-debug.tar.gz
bfbbb20dc4e7b30444a52f5f57b5789b5d1edee80abdc8066129b48c59ee65c9  guix-build-d873ff96e51a/output/powerpc64le-linux-gnu/bitcoin-d873ff96e51a-powerpc64le-linux-gnu.tar.gz
65d578b81b00a1032039362dc6be1a71368f390188e0f948829afd03b8858ed2  guix-build-d873ff96e51a/output/riscv64-linux-gnu/SHA256SUMS.part
e5233d7e7a8832893ff414c78eb3d4bca3ae30d1a1f789a23419c6739b203022  guix-build-d873ff96e51a/output/riscv64-linux-gnu/bitcoin-d873ff96e51a-riscv64-linux-gnu-debug.tar.gz
fb6d9f5a063dc7752fcc2acc95a0052322d7c8c86d2c6373e0ceb949dcf22f49  guix-build-d873ff96e51a/output/riscv64-linux-gnu/bitcoin-d873ff96e51a-riscv64-linux-gnu.tar.gz
61c89850244ddf5813ff80c242eff89925d30bccadfa5cb63e968c3af49eb964  guix-build-d873ff96e51a/output/x86_64-apple-darwin/SHA256SUMS.part
cd219fab8918b061a342357d298aca0c044feb34c6d50a7851d5d3bf18cec267  guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin-unsigned.dmg
1170d3fdb199fbfca2c20b2a77cc81a6fe24b7e4973543a4461e887f14ac68e9  guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin-unsigned.tar.gz
71e93297ed8c581a7ed32a6948ef7b1ea2e7c43cb054181de3b5f604f7a2c28b  guix-build-d873ff96e51a/output/x86_64-apple-darwin/bitcoin-d873ff96e51a-x86_64-apple-darwin.tar.gz
46e9b067ec385ee14642aebc5ec09d7d2382e0204eeb17dc64587013eddd5dff  guix-build-d873ff96e51a/output/x86_64-linux-gnu/SHA256SUMS.part
23278b19daac51e7df65b817b79fc93562d0f4eb193ef87472456f4bed1464d7  guix-build-d873ff96e51a/output/x86_64-linux-gnu/bitcoin-d873ff96e51a-x86_64-linux-gnu-debug.tar.gz
4d5e5e23f089a59185f62faf367d8ca86476e406e6b7bbc9e8950cd89d94534d  guix-build-d873ff96e51a/output/x86_64-linux-gnu/bitcoin-d873ff96e51a-x86_64-linux-gnu.tar.gz
eec8ab97ee9aceef8cb4e7cb5026225ffc5c7b8e8a6d376e8348020000e5af88  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/SHA256SUMS.part
a31819e67c373f30eafce8dbcb3d6d0c61d1dcf59c51023aa79321934f8a7d2a  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-debug.zip
2e7d4e533a5998863c115c586c61b75b4039cd329e12ed24cff78b7f16b6ea57  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-setup-unsigned.exe
3dabbd627b532beef57c3d4b5bd30c93c5ea74c492918484cf24685aca8d7bc4  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64-unsigned.tar.gz
ec438531b4694913dbbf7c91920dcbd957354b164f807867c16a001898edf669  guix-build-d873ff96e51a/output/x86_64-w64-mingw32/bitcoin-d873ff96e51a-win64.zip
  2. laanwj commented at 12:03 PM on June 14, 2022: member

    Concept ACK. In a way this feels like scope creep. Why the hell would we want to maintain a JSON library?

    But given how far we've diverged from upstream univalue, and upstream library hardly being maintained, this already has been effectively the case for a while. This just removes some extra steps.

  3. MarcoFalke commented at 12:27 PM on June 14, 2022: member

    Concept ACK.

    Apart from simplifying our workflow, this also clarifies that the univalue lib in its current form is unmaintained and any effort to try supporting it as a system library is a largely wasted effort.

    I can see the reason for using a system library for json stuff, but ideally it should be something that is maintained. Looks like there is https://github.com/cculianu/univalue which could serve as a drop-in, but I am not sure how much it is maintained, how much it is available as a system package, how much effort it would be to switch to it and if it is even possible to use as a drop-in replacement, considering their and our past changes.

    There is also https://github.com/nlohmann/json, which is not a library, but a single header file. So I am actually thinking it wouldn't be too much hassle to make our univalue a single C++17 header file as well. At which point it could be trivially copied by other projects (or trivially distributed as a "library"), if needed. Though, that seems best to be discussed in a separate thread (or maybe not at all, since it is out-of-scope?).

  4. laanwj commented at 1:00 PM on June 14, 2022: member

    If we only used univalue for JSON parsing it'd be fairly straightforward to switch to a better supported library. However, our entire RPC layer uses it as data representation. So I think it would be too much impact to switch. (Sure, could add some conversion layer that abstracts and keeps the internal interface the same, but then, univalue works pretty well for our purposes i'm sure a new library would have all kind of switching costs in the form of bugs and unexpected behavior)

  5. DrahtBot added the label Refactoring on Jun 14, 2022
  6. laanwj added the label RPC/REST/ZMQ on Jun 14, 2022
  7. fanquake force-pushed on Jun 14, 2022
  8. sipa commented at 1:58 PM on June 14, 2022: member

    univalue works pretty well for our purposes i'm sure a new library would have all kind of switching costs in the form of bugs and unexpected behavior)

    Don't forget that before univalue, we used another library (JSON Spirit). And the switch to Univalue was made specifically because it was designed for a purpose that most JSON libraries (at the time, at least) didn't provide: exact roundtripping of the string representation of values (so that it would be possible to use custom encodings and parsings of floating-point values, bypassing the double representation that would imply rounding). If we'd want another library, that would be a requirement, I think.

    Concept ACK. The reality is that the current univalue codebase we're using is effectively maintained by us already, and we're relying on many improvements that the original upstream doesn't have. I'd be open to discussing alternatives, as I agree an externally maintained library is certainly better if a suitable candidate exists, but that seems like an independent discussion from this PR.

  9. fanquake commented at 2:38 PM on June 14, 2022: member

    I've fixed the CI (make dist) and added Guix builds to the op.

  10. laanwj commented at 3:03 PM on June 14, 2022: member

    Don't forget that before univalue, we used another library (JSON Spirit).

    I sure remember, I still remember how much of a pain it was to change over to univalue, we had all kinds of bugs for many releases. I just don't think it's a good use of time to do that now again. Do we have any issues with univalue?

  11. sipa commented at 3:11 PM on June 14, 2022: member

    I sure remember, I still remember how much of a pain it was to change over to univalue, we had all kinds of bugs for many releases. I just don't think it's a good use of time to do that now again. Do we have any issues with univalue?

    Sorry, I didn't mean to argue in favor of moving away from univalue; quite the opposite. It works fine for our purposes (as it was specifically designed even to cover one of our rather unusual requirements). The only downside is that it requires us to maintain it going forward, but if we're ok with that - no concern.

    The only point I wanted to make is that the discussion about whether or not to de-subtree univalue is independent from the discussion about moving to another library altogether.

  12. luke-jr changes_requested
  13. luke-jr commented at 4:19 PM on June 14, 2022: member

    Concept NACK. The goal should be for more modularity, not less. There is no reason we should be maintaining a dedicated JSON library in this project. True, upstream has problems, but this PR is a step in the wrong direction.

  14. theuni commented at 4:33 PM on June 14, 2022: member

    Concept ACK. We effectively maintain this anyway, so we may as well maintain it conveniently. I think we'll want to get away from Univalue eventually, but I agree with @sipa, that's not what this is about.

    Modularity is unaffected. It exports the same interface as before.

  15. Sjors commented at 6:50 PM on June 14, 2022: member

    Concept ACK. There is a trade-off between being modular and maintenance burden. The most extreme example of that trade-off in action is the Node Package Manager ecosystem. We too could split off individual classes in src/crypto into their own subtrees or submodules. Some of those, like MuHash, ChaCha20 and hand crafted assembly SHA256 may even have more potential users than UniValue.

    If there was a more convenient way to have individual git repos for tightly integrated small components like that, that would be nice. Then we could revisit what to do with UniValue.

  16. in src/univalue/gen/gen.cpp:8 in 1271737355 outdated 
       0 | @@ -1,84 +0,0 @@
   1 | -// Copyright 2014 BitPay Inc.
   2 | -// Distributed under the MIT software license, see the accompanying
   3 | -// file COPYING or https://opensource.org/licenses/mit-license.php.
   4 | -
   5 | -//
   6 | -// To re-create univalue_escapes.h:
   7 | -// $ g++ -o gen gen.cpp
   8 | -// $ ./gen > univalue_escapes.h
    MarcoFalke commented at 6:00 AM on June 15, 2022:

    I wonder if this file should stay? I know it is unused by the build system, but it may be used by devs (in the unlikely case that the header needs to be re-generated)?

    fanquake commented at 8:41 AM on June 15, 2022:

    If someone wants to regenerate the header (it hasn't changed in the last 7 years besides the NULL -> nullptr refactor), I think they could retreive the file from git. I'd rather not keep un-(likely-to-be)-used/tested files around. If the header needs to be changed, it could also just be modified directly.

    fanquake commented at 9:03 AM on June 15, 2022:

    continues in #25369 (review)

  17. in .gitignore:48 in 46e0aaf3e0 outdated 
      43 | @@ -44,8 +44,6 @@ src/obj
  44 |  share/setup.nsi
  45 |  share/qt/Info.plist
  46 |  
  47 | -src/univalue/gen
  48 | -
    MarcoFalke commented at 6:01 AM on June 15, 2022:

    Same

    fanquake commented at 8:41 AM on June 15, 2022:

    Commented above

  18. in src/univalue/include/univalue.h:174 in 46e0aaf3e0 outdated 
     170 | @@ -170,8 +171,7 @@ static inline bool jsonTokenIsValue(enum jtokentype jtt)
 171 |      default:
 172 |          return false;
 173 |      }
 174 | -
 175 | -    // not reached
 176 | +    assert(false);
    MarcoFalke commented at 6:02 AM on June 15, 2022:

    Any reason to silence compiler warnings in a refactoring commit?

    fanquake commented at 8:56 AM on June 15, 2022:

    Can be a separate commit if you'd prefer

    MarcoFalke commented at 9:00 AM on June 15, 2022:

    I'd prefer to not change it at all, unless there is a reason to?

    fanquake commented at 9:02 AM on June 15, 2022:

    Will drop

  19. in src/univalue/include/univalue.h:189 in 46e0aaf3e0 outdated 
     185 | @@ -186,12 +186,11 @@ static inline bool json_isspace(int ch)
 186 |      default:
 187 |          return false;
 188 |      }
 189 | -
 190 | -    // not reached
 191 | +    assert(false);
    MarcoFalke commented at 6:03 AM on June 15, 2022:

    Same

    fanquake commented at 9:02 AM on June 15, 2022:

    see above

  20. in test/lint/lint-includes.py:21 in 46e0aaf3e0 outdated 
      17 | @@ -18,8 +18,7 @@
  18 |  EXCLUDED_DIRS = ["src/leveldb/",
  19 |                   "src/crc32c/",
  20 |                   "src/secp256k1/",
  21 | -                 "src/minisketch/",
  22 | -                 "src/univalue/"]
  23 | +                 "src/minisketch/"]
    MarcoFalke commented at 6:05 AM on June 15, 2022:

    Why remove the trailing comma?

    fanquake commented at 8:52 AM on June 15, 2022:

    Re-added

  21. in src/univalue/test/unitester.cpp:16 in 46e0aaf3e0 outdated 
      11 | @@ -12,16 +12,9 @@
  12 |  #error JSON_TEST_SRC must point to test source directory
  13 |  #endif
  14 |  
  15 | -#ifndef ARRAY_SIZE
  16 | -#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
  17 | -#endif
  18 | -
  19 |  std::string srcdir(JSON_TEST_SRC);
  20 |  static bool test_failed = false;
    MarcoFalke commented at 6:06 AM on June 15, 2022:

    Forgot to remove this?

    fanquake commented at 8:41 AM on June 15, 2022:

    Fixed in next push

  22. MarcoFalke approved
  23. MarcoFalke commented at 6:06 AM on June 15, 2022: member

    almost lgtm

  24. DrahtBot commented at 8:30 AM on June 15, 2022: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25039 (lint: convert git-subtree-check.sh to Python by jacobpfickes)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  25. refactor: un-subtree univalue
    Remove all the files we don't use.
This should not change behavior, or anything at all, as none of these
files are currently used in our build system.
    e2aa7047f9
  26. in src/univalue/include/univalue_escapes.h:1 in 46e0aaf3e0 outdated 
       0 | @@ -1,6 +1,6 @@
   1 |  // Automatically generated file. Do not modify.
    MarcoFalke commented at 8:45 AM on June 15, 2022:

    Well, then maybe leave a comment along the lines of:

    Generator removed in commit 1271737355542049eb9e368174903af379106ffd

    fanquake commented at 8:46 AM on June 15, 2022:

    I'll just remove the // Automatically generated file. Do not modify. comment entirely.

  27. fanquake force-pushed on Jun 15, 2022
  28. in src/univalue/include/univalue_escapes.h:2 in c673e1dbad outdated 
       0 | @@ -1,6 +1,5 @@
   1 | -// Automatically generated file. Do not modify.
   2 | -#ifndef BITCOIN_UNIVALUE_UNIVALUE_ESCAPES_H
   3 | -#define BITCOIN_UNIVALUE_UNIVALUE_ESCAPES_H
   4 | +#ifndef BITCOIN_UNIVALUE_INCLUDE_UNIVALUE_ESCAPES_H
   5 | +#define BITCOIN_UNIVALUE_INCLUDE_UNIVALUE_ESCAPES_H
    MarcoFalke commented at 9:33 AM on June 15, 2022:

    I guess moving the files is required for build system reasons?

    If not, I'd also prefer to leave the location as-is, unless there is a reason to change it.

    fanquake commented at 9:46 AM on June 15, 2022:

    It's required if we want to have src/univalue integrated / linted as-if it's part of of this codebase. Otherwise we'd have to retain exceptions to at least the include linter, which would be somewhat confusing given it's no-longer a subtree.

  29. in src/univalue/include/univalue.h:9 in c673e1dbad outdated 
       2 | @@ -3,9 +3,10 @@
   3 |  // Distributed under the MIT software license, see the accompanying
   4 |  // file COPYING or https://opensource.org/licenses/mit-license.php.
   5 |  
   6 | -#ifndef __UNIVALUE_H__
   7 | -#define __UNIVALUE_H__
   8 | +#ifndef BITCOIN_UNIVALUE_INCLUDE_UNIVALUE_H
   9 | +#define BITCOIN_UNIVALUE_INCLUDE_UNIVALUE_H
  10 |  
  11 | +#include <cassert>
    MarcoFalke commented at 11:16 AM on June 15, 2022:

    what is this?

    fanquake commented at 11:18 AM on June 15, 2022:

    leftover from previous change. removed

  30. fanquake force-pushed on Jun 15, 2022
  31. in src/univalue/test/unitester.cpp:154 in b7918b86fa outdated 
     158 |  }
 159 |  
 160 |  int main (int argc, char *argv[])
 161 |  {
 162 | -    for (unsigned int fidx = 0; fidx < ARRAY_SIZE(filenames); fidx++) {
 163 | +    for (unsigned int fidx = 0; fidx < std::size(filenames); fidx++) {
    MarcoFalke commented at 11:20 AM on June 15, 2022:

    nit: Could use a C++11 range-loop instead?

    fanquake commented at 11:58 AM on June 15, 2022:

    Added

  32. MarcoFalke approved
  33. MarcoFalke commented at 11:20 AM on June 15, 2022: member

    ACK c673e1dbad8f532052827f05045695c8717c32b2 🔬

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK c673e1dbad8f532052827f05045695c8717c32b2 🔬
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjpeQv/Tnz8e79UCsuOokHrmC/5gPv1bQQ86m+5CDXPCZJDx2C6iG+H593QsHSZ
QTW4Z2lKTTtBdMp+bUY739MrmiIBBcx2naSyBeoBY1Sh4e/C+5C1MZ3ZvYpIVj0T
Oqv99WX5IbbjyYpPueyzbo49MEF5yem6lpmK3POxn0rst6FOh3GeSVJ3OezDZmHR
ECWSnGXa+SLUdwYwQ4AHsC0hEs/75X0sIqqz/9QKBtdyXzfdDXak0KURfXxbwE0b
jEePPiDwrpOLyEbKcpy9qQyCrk9E+mILmOv9O49wHhNo4X1j1YV+jFLNDuZHmPqk
9JQhOOR7RUF82HEZ/c5E//mZM9Ijm+1zVx0tYqWOXS2ql+1J/lsYBDpRUj/KfHIo
gCE4bAorwPAhLI1VOfFFJoRE99tuXtTKgyUXTDXFLRtKWq4pAkNQOmGXgZqSk/Zz
1poBqvkDK6IREspH2dCD9mWj0QrRj8yMFCWT4WrWxTYVfQ0oMRG6VJ2aP4hX22JT
0idJ4kyQ
=qAvf
-----END PGP SIGNATURE-----

    </details>

  34. fanquake force-pushed on Jun 15, 2022
  35. refactor: cleanups post unsubtree'ing univalue
    Mostly changes to remove src/univalue exceptions from the various linters,
and the required code changes to make them happy. As well as minor doc
changes.
    d873ff96e5
  36. fanquake force-pushed on Jun 15, 2022
  37. laanwj commented at 9:08 AM on June 16, 2022: member

    Code review ACK d873ff96e51a3e7f2fdc3fdd1baee2bbe7583e06

  38. MarcoFalke commented at 11:46 AM on June 16, 2022: member

    re-ACK d873ff96e51a3e7f2fdc3fdd1baee2bbe7583e06 only changes: 📼

    • Removing accidentally added unused include
    • Use range-based for-loop over std::size

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK d873ff96e51a3e7f2fdc3fdd1baee2bbe7583e06 only changes: 📼
* Removing accidentally added unused include
* Use range-based for-loop over std::size
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiyEwv+KlCi0NtPaByrp//3nAYFCvD14nUkwz/rhuwiRoojRIazMjBWF/kDzGX9
9KSGHcMXn2TxALWrnbAv6ugULtflrpwS5dYCMyhuk9dniOpm4ETnqT8U0GA8PLpo
wyKOz++7n9jUR7XSaFWXJnpjj0JshxkBQ2r9opFG8KMPOa4SyY+V1DH9LpCqDBlN
d7Qm0QLk0irpbxU4dWV+mzpAjif/Ig3n0yC1dRpOq/nGEyv2kSuauLiTN82k71A8
7hp2P0rZl5VyjZpcNIOY5kealqAEIhApY1rqBufcp6t0TiaR6l4iWBHmNLeNa+d5
HCMLDSUIOzJPYqeJH7c68PK+Lww0YgjOIWR44raSzYWbv59GNvnB3cOTBgMr9UX3
yaSSbXmUh1o5h/7a2+uXlW/kNhSqyCKNQVs0ia8z8cjJA0XU+FdU49kVbxs4HX3E
ewHteAT/2/csRs9puD4yUFgRHWybxR7vXFowHFLoJ0c9PAWUjzzLM5QoscWgPXoP
phNTtkXO
=HpN+
-----END PGP SIGNATURE-----

    </details>

  39. MarcoFalke merged this on Jun 16, 2022
  40. MarcoFalke closed this on Jun 16, 2022
  41. fanquake deleted the branch on Jun 16, 2022
  42. sidhujag referenced this in commit 83f01fd4e1 on Jun 16, 2022
  43. DrahtBot locked this on Jun 16, 2023
Contributors
fanquakelaanwjMarcoFalkesipaluke-jrtheuniSjorsDrahtBot
Labels
RefactoringRPC/REST/ZMQ
Linked (view graph)
#6637 Add UniValue as subtree#7349 Build against system UniValue when available#22646 build: tighter Univalue integration, remove `--with-system-univalue`
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me