Revert "build: Use Homebrew's sqlite package if it is available"

fanquake commented at 2:04 pm on September 2, 2022: member

This reverts ee7b84e63cbeadd5e680d69ff0548275581e9241 from #20527.

That change was made without any rationale, maybe other than, a brew installed version might be newer, and that’s “better”. However when building from source on macOS, it just results in drastically worse performance, and issues / confusion like #25724.

The difference in performance can be observed using the example from #25724 (comment), but minified i.e:

 0time src/bitcoin-cli createwallet speedy true
 1time src/bitcoin-cli importdescriptors '[
 2  {"desc":"raw(00145846369f3d6ba366d6f5a903fb5cf4dca3763c0e)#k9wh6v62","timestamp":"now"},
 3  {"desc":"raw(001420800aabf13f3a4c4ce3ce4c66cecf1d17f21a6e)#6m0hlfh4","timestamp":"now"},
 4  {"desc":"raw(0014c6bf9715e06d73ebf9b3b02d5cc48d24d8bbabc1)#wyavh36r","timestamp":"now"},
 5  {"desc":"raw(00141ba7807b3f46af113beaea5c698428ce7138cd8a)#jctdsups","timestamp":"now"},
 6  {"desc":"raw(00140c1bd27f10fff01b36ddf3c1febaa1acff19b080)#9s6nc3pk","timestamp":"now"},
 7  {"desc":"raw(00141226e31987e4bc2e63c0ee12908f675e40464b20)#9pp7qm39","timestamp":"now"},
 8  {"desc":"raw(0014f73f149f7503960a5e849c6ee7a8a8c336f631cb)#qtkxv9fc","timestamp":"now"},
 9  {"desc":"raw(0014c8ccb4d81ffc769fc5fdd8d7eed69b0e0cae5749)#hn39qayv","timestamp":"now"},
10  {"desc":"raw(001498565aead2d67a22a6021d55210f2a917fc22169)#6ar3vwsx","timestamp":"now"},
11  {"desc":"raw(001403013248ac0cd9eabe176cad162cda2a19f771e1)#4m47mukd","timestamp":"now"}
12]'

Running master, when building from souce and using brew installed sqlite, this takes ~3.4s. With this PR, the same operation takes ~0.3s.

Resolves the “build from source” portion of #25724. Building from depends is still not ideal, however I have some other changes that might help improve things in that case.

Related performance issue reports:

Revert "build: Use Homebrew's sqlite package if it is available"

This reverts ee7b84e63cbeadd5e680d69ff0548275581e9241 from #20527.
This change was made without any rationale, maybe other than a brew
installed version might be newer, and that's "better". However when
building from source on macOS, it just results in drastically worse
perofrmance, and results in issues / confusions like #25724.

Resolves the "build from source" portion of #25724. Building from
depends is still not ideal, however I have some other changes that might
help improve things in that case.

The difference in performance can be observed using the example from
https://github.com/bitcoin/bitcoin/issues/25724#issuecomment-1213554922,
but minified to only 10 descriptors. i.e:
```bash
time src/bitcoin-cli createwallet speedy true
time src/bitcoin-cli importdescriptors '[
  {"desc":"raw(00145846369f3d6ba366d6f5a903fb5cf4dca3763c0e)#k9wh6v62","timestamp":"now"},
  {"desc":"raw(001420800aabf13f3a4c4ce3ce4c66cecf1d17f21a6e)#6m0hlfh4","timestamp":"now"},
  {"desc":"raw(0014c6bf9715e06d73ebf9b3b02d5cc48d24d8bbabc1)#wyavh36r","timestamp":"now"},
  {"desc":"raw(00141ba7807b3f46af113beaea5c698428ce7138cd8a)#jctdsups","timestamp":"now"},
  {"desc":"raw(00140c1bd27f10fff01b36ddf3c1febaa1acff19b080)#9s6nc3pk","timestamp":"now"},
  {"desc":"raw(00141226e31987e4bc2e63c0ee12908f675e40464b20)#9pp7qm39","timestamp":"now"},
  {"desc":"raw(0014f73f149f7503960a5e849c6ee7a8a8c336f631cb)#qtkxv9fc","timestamp":"now"},
  {"desc":"raw(0014c8ccb4d81ffc769fc5fdd8d7eed69b0e0cae5749)#hn39qayv","timestamp":"now"},
  {"desc":"raw(001498565aead2d67a22a6021d55210f2a917fc22169)#6ar3vwsx","timestamp":"now"},
  {"desc":"raw(001403013248ac0cd9eabe176cad162cda2a19f771e1)#4m47mukd","timestamp":"now"}
]'
```

Running master, when building from souce and using brew installed
sqlite, this takes ~3.4s. With this PR, the same operation takes ~0.3s.

d216d714aa

fanquake added the label macOS on Sep 2, 2022

fanquake added the label Build system on Sep 2, 2022

maflcko added this to the milestone 24.0 on Sep 2, 2022

maflcko added the label Needs backport (22.x) on Sep 2, 2022

maflcko added the label Needs backport (23.x) on Sep 2, 2022

achow101 commented at 4:31 pm on September 2, 2022: member

It would be nice if we could figure out why macOS SQLite is different from self built so that we can replicate those performance improvements in our depends build.

michaelfolkson commented at 5:03 pm on September 2, 2022: contributor

Concept ACK

The Homebrew docs recommend to “not remove macOS native tools and forcefully replace them with symlinks back to the Homebrew-provided tool.”

But I couldn’t find anything on whether MacOS’s version of SQLite is different to the brew version of SQLite (assuming the same version numbers). That difference in performance is surely not entirely explained by different version numbers being used.

jarolrod commented at 8:08 pm on September 2, 2022: member

Concept ACK

it’s not correct to say there was “no rationale”, it’s in the pr description:

On master (https://github.com/bitcoin/bitcoin/commit/7ae86b3c6845873ca96650fc69beb4ae5285c801) installed Homebrew sqlite package is ignored during build on macOS. This PR fixes this issue and update macOS build docs.

But we now know we’d prefer not to use the brew version.

fanquake commented at 8:24 pm on September 2, 2022: member

It would be nice if we could figure out why macOS SQLite is different from self built so that we can replicate those performance improvements in our depends build.

I agree. Started having a look through https://github.com/apple-oss-distributions to see if there might be any clues.

it’s not correct to say there was “no rationale”, it’s in the pr description:

The description just says we should prefer brew installed sqlite if it happens to be installed. There’s no reasoning given as to why we should prefer that instead of just using the system library (i.e it’s faster, has newer features we require, system libs are severely outdated/buggy) etc.

hebasto commented at 7:05 am on September 3, 2022: member

What is the sqlite version on macOS 10.15?

Sjors commented at 11:31 am on September 5, 2022: member

Tested d216d714aae36e6f1c95f82aef81a0be74dee2f3

On the first machine the system library is twice as fast, on the second machine there’s no difference (or it’s even slightly slower). I checked in the log that right Sqlite version was indeed used.

I agree with @hebasto that we should check if this holds for older (supported) macOS versions. Perhaps a safer approach is to pick whichever version is higher.

Machine 1 MacBook Pro (2.3 GHz 8-Core Intel Core i9), macOS 12.5.1

with Sqlite 3.39.2 via homebrew: 3.8s
system Sqlite 3.37.0: 2.0s

Machine 2: iMac (3.4 GHz Quad-Core Intel Core i5): macOS 12.5.1

with Sqlite 3.39.2 via homebrew: 0.5s
system Sqlite 3.37.0: 0.6s

Note that machine 2 is much slower than machine 1, yet sqlite performs faster. There’s something else going on, possibly specific to my setup. I got even more extreme results with the migratewallet RPC.

fanquake commented at 2:43 pm on September 5, 2022: member

Perhaps a safer approach is to pick whichever version is higher.

NACK. More often than not, that would just result in the situation where we get worse performance. We aren’t using features from newer versions, and Apple already backports bug and security fixes to the system libs.

fanquake commented at 3:27 pm on September 8, 2022: member

What is the sqlite version on macOS 10.15?

Looks like it is 3.28.0+.

luke-jr commented at 4:13 am on September 10, 2022: member

So this is preferring the macOS/native sqlite over the Homebrew sqlite, right? (I assume we already prefer depends-built if it’s been made available.)

Can the user still explicitly override it and build with Homebrew’s if they so choose? (--with-sqlite=/some/path?)

fanquake commented at 10:32 am on September 10, 2022: member

So this is preferring the macOS/native sqlite over the Homebrew sqlite, right?

Yes. See also #25724.

(I assume we already prefer depends-built if it’s been made available.)

If by “made available”, you mean a CONFIG_SITE/--prefix was passed to configure, then yes.

Can the user still explicitly override it and build with Homebrew’s if they so choose?

Yes. The user can ultimately still choose whichever sqlite they prefer. i.e set LDFLAGS & CPPFLAGS to point to brews sqlite, and that will be used.

jonatack commented at 11:32 am on September 10, 2022: contributor

Not sure if this issue applies to arm64 (intel only) or if I’m testing incorrectly. Installed sqlite3 from homebrew and I’m not seeing a difference between master and this branch on a MacBook Pro 16 (M1 Max), macOS 12.3. In both cases the example takes 5 seconds. Tested back and forth a few times.

 0$  clang -v
 1Apple clang version 13.1.6 (clang-1316.0.21.2.5)
 2Target: arm64-apple-darwin21.4.0
 3Thread model: posix
 4InstalledDir: /Library/Developer/CommandLineTools/usr/bin
 5
 6$ ./configure --with-incompatible-bdb --enable-suppress-external-warnings CC=clang CXX=clang++
 7
 8$ ./src/bitcoin-cli -rpcwallet=speedy importdescriptors   0.00s user 0.00s system 0% cpu 5.293 total
 9
10$ sqlite3 --version
113.37.0 2021-12-09 01:34:53 9ff244ce0739f8ee52a3e9671adb4ee54c83c640b02e3f9d185fd2f9a179aapl

jonatack commented at 11:34 am on September 10, 2022: contributor

Ah, the system sqlite3 3.37 was being used in either case. Needed to restart the terminal to pick up the link (echo 'export PATH="/opt/homebrew/opt/sqlite/bin:$PATH"' >> ~/.zshrc) to sqlite3 3.39.3 2022-09-05, after which both master and this branch run the example rpc after configure + make in around half a second (macOS Monterey 12.3).

Sjors commented at 4:23 pm on September 12, 2022: member

Just tried (the latest master) with macOS 10.15 Catalina, freshly installed on a 2015 Macbook Pro, Intel i5 dual core:

with Sqlite 3.39.3 via homebrew: 1.6s
system Sqlite 3.28.0: 1.6s

(caveat: I only synced a fraction of mainnet and then paused it, so the rescan part is a bit faster: 1ms on the 2015 machine vs 423ms on the 2019 machine)

fanquake commented at 10:00 am on September 13, 2022: member

I’m not sure what to make of the benchmarking / results being posted here. I think in some cases not quite the right thing is being tested, or results are being mixed up. However the general case still seems to be that the brew libs are either the same speed, or much slower than the system libs.

I haven’t seen an argument for always prefering the brew libs, and in that case you’re just rolling the dice for much worse performance, and ending up with #25724.

Note that the benchmarking can also be performed without using brew at all. You can just compare a build with system libs, vs a depends build, as sqlite as compiled via brew, is going to be ~the same as the lib you get when compiling in depends.

Looking at the macOS sqlite system lib, it’s pretty clear that it contains code that isn’t in the open source sqlite. There are a number of additional symbols, including __sqlite3_apple_archive, __sqlite3_apple_archive_type, __sqlite3_apple_unarchive, __sqlite3_purgeEligiblePagerCacheMemory etc. As well as _sqlite3_key, _sqlite3_key_v2, which indicate the presence of non-free sqlite extensions, such as The SQLite Encryption Extension.

Sjors commented at 1:26 pm on September 13, 2022: member

However the general case still seems to be that the brew libs are either the same speed, or much slower than the system libs.

That’s what I was testing.

Concept ACK

The presence of a Homebrew version doesn’t mean the user intended it for Bitcoin. Relying on ~closed source~ non free Apple stuff that’s sometimes faster, but in (so far) unpredictable ways, doesn’t seem very appealing either.

But the fact that the non-homebrew version sometimes is dramatically faster, is probably a good enough reason to prefer it.

Christewart commented at 8:52 pm on September 13, 2022: contributor

This seems to fix my performance issues on my m1 mac when running tests against bitcoind.

This is on the official v23 release (36 seconds)

This is on d216d714aae36e6f1c95f82aef81a0be74dee2f3 (!5 seconds!)

So this is a dramatic performance improvement for our test harness!

In case anyone wants stack exchange reputation, consider answering this question: https://bitcoin.stackexchange.com/questions/113898/bitcoin-v23-is-10-times-slower-than-v22-on-macos-for-basic-regtest-tests

fanquake commented at 9:59 am on September 14, 2022: member

This seems to fix my performance issues on my m1 mac when running tests against bitcoind.

Thanks for confirming this fixes the performance issues you’ve been seeing. While the release binaries still wont be “fixed”, if/when this is merged, the situation for compiling locally / development should be much improved.

In case anyone wants stack exchange reputation, consider answering this question: https://bitcoin.stackexchange.com/questions/113898/bitcoin-v23-is-10-times-slower-than-v22-on-macos-for-basic-regtest-tests

I’m mostly interested in running integration tests for a project and it looks like I cannot upgrade to the new version yet without solving this.

Good to know that they were using x86_64, so we can further rule out an x86_64 vs arm64 (M1) difference. The fact that developers are landing on the solution of “not upgrading”, is another reason to fix this.

achow101 commented at 9:24 pm on September 15, 2022: member

ACK d216d714aae36e6f1c95f82aef81a0be74dee2f3

I also observe a significant performance improvement with this.

hebasto commented at 7:38 am on September 16, 2022: member

This reverts ee7b84e from #20527. That change was made without any rationale

That is untrue. Besides #20527 description. one could refer #20498. Build docs implied that Bitcoin Core uses Homebrew’s sqlite package. And #20527 made it explicit.

I’d say the opposite: this reversion is unjustified.

Relying on Apple’s sqlite package undermines Bitcoin Core security.

when building from source on macOS, it just results in drastically worse performance

Then such performance issue should be fixed in Homebrew’s sqlite package.

The user can ultimately still choose whichever sqlite they prefer. i.e set LDFLAGS & CPPFLAGS to point to brews sqlite, and that will be used.

If users want to use Apple’s sqlite, they can use the same approach. No code change is required at all.

NACK from me.

fanquake commented at 8:09 am on September 16, 2022: member

Besides #20527 description. one could refer #20498.

There is nothing in either of these threads that gives reasoning for why a brew installed sqlite should be preferred. #20498 just seems to be figuring out if macOS comes with a sqlite system library.

Relying on Apple’s sqlite package undermines Bitcoin Core security.

How so? This isn’t being used in the release binaries. It’s for developers/users self-compiling on macos. Self compilation / developer envinronments already come with lessened security assumptions, because your dependencies are coming from third party package managers, the OS, etc.

Then such performance issue should be fixed in Homebrew’s sqlite package.

What you mean? Homebrew just compiles the unmodified sqlite source, there is nothing to “fix” there. Fixing this likely requires upstreaming changes to sqlite. If we actually had those changes, we could also be applying them in depends (and fixing this for releases).

If users want to use Apple’s sqlite, they can use the same approach. No code change is required at all.

So you suggest we leave this as-is, and just continually tell downstream projects / confused devs that to get reasonable performance in their developer environments, they have to compile differently, and avoid homebrews sqlite, even though our build system is setup to prefer it by default?

Here’s another instance of a project, this time BDK, running into this same problem: https://github.com/bitcoindevkit/bdk/issues/749.

hebasto commented at 8:20 am on September 16, 2022: member

Relying on Apple’s sqlite package undermines Bitcoin Core security.

How so?

Like that:

Looking at the macOS sqlite system lib, it’s pretty clear that it contains code that isn’t in the open source sqlite.

If users want to use Apple’s sqlite, they can use the same approach. No code change is required at all.

So you suggest we leave this as-is, and just continually tell downstream projects / confused devs that to get reasonable performance in their developer environments, they have to compile differently, and avoid homebrews sqlite, even though our build system is setup to prefer it by default?

Our defaults must prioritize security, not performance.

To avoid confusing, using of an Apple’s library can be provided as an example in our docs.

fanquake commented at 8:38 am on September 16, 2022: member

Like that:

Can you clarify what you mean by security? I don’t quite understand why we must avoid the system sqlite lib at all costs, while at the same time we are going to happily load any other number of apple shared libs (which are complete black boxes) at runtime (also true for release binaries).

Our defaults must prioritize security, not performance. To avoid confusing, using of an Apple’s library can be provided as an example in our docs.

In our release binaries, I agree. However, self-compilation on macOS, is almost always going to be a developer environment, which as I mentioned above, already comes with lessened security assumptions.

In this case, from a security perspective, why is using Apples sqlite dylib, any worse than using homebrews? In both cases, you’re using a precompiled library, provided by a third party. Are homebrews prebuilt libs even deterministic/reproducible? (I know you can tell brew to build from source, but 99.9% of users installing sqlite from brew are not doing that)?

hebasto commented at 8:58 am on September 16, 2022: member

Can you clarify what you mean by security?

Invulnerability. As much as possible.

For instance, did someone test that Apple’s system library won’t crash Bitcoin Core in corner cases?

Every macOS update can potentially lead to any unpredictable consequences.

I don’t quite understand why we must avoid the system sqlite lib at all costs, while at the same time we are going to happily load any other number of apple shared libs (which are complete black boxes) at runtime (also true for release binaries).

Do we have open-source alternatives for other Apple’s library?

My question is similar: why we must use Apple’s modified version of an open-source library?

However, self-compilation on macOS, is almost always going to be a developer environment

Correct. That is why just to document usage of a system library is enough.

In this case, from a security perspective, why is using Apples sqlite dylib, any worse than using homebrews? In both cases, you’re using a precompiled library, provided by a third party. Are homebrews prebuilt libs even deterministic/reproducible? (I know you can tell brew to build from source, but 99.9% of users installing sqlite from brew are not doing that)?

Apple even doesn’t provide such an opportunity.

luke-jr commented at 8:29 pm on September 16, 2022: member

Self compilation / developer envinronments already come with lessened security assumptions, because your dependencies are coming from third party package managers, the OS, etc.

Self-compilation is the ideal / best practice for security. There is no reason to assume these are developer environments, and strong NACK on doing anything that would compromise self-compilation as the recommended ideal for end users / production usage.

The OS is always inherently trusted. Static linking is a reduction in security as a tradeoff. (But because the OS is always trusted, using the OS sqlite by default does make sense.)

So you suggest we leave this as-is, and just continually tell downstream projects / confused devs that to get reasonable performance in their developer environments,

The real question here is, why is upstream sqlite performance such a problem?

Sure, maybe Apple has an optimised version, but stock sqlite should perform well enough, right?

achow101 commented at 11:18 pm on September 16, 2022: member

Sure, maybe Apple has an optimised version, but stock sqlite should perform well enough, right?

It should, but it doesn’t, as measured using the example in the OP. There’s a 10x performance speedup from just switching from homebrew/depends/self-compiled sqlite to system sqlite, at least on x86 Macs.

luke-jr commented at 11:49 pm on September 16, 2022: member

Is ~3.4s really so bad for importing a bunch of descriptors?

achow101 commented at 0:38 am on September 17, 2022: member

Is ~3.4s really so bad for importing a bunch of descriptors?

Well it gets worse as more are imported at the same time. The original example was larger and took 30 seconds vs 3 seconds. The problem is also that downstream software which imports many descriptors may run into RPC client timeouts (although arguably that’s a problem with their RPC client). Also this performance issue effects other things too, although they are less directly measurable.

luke-jr commented at 4:55 pm on September 17, 2022: member

Possibly related: https://bonsaidb.io/blog/acid-on-apple/

achow101 commented at 6:02 pm on September 17, 2022: member

Possibly related: https://bonsaidb.io/blog/acid-on-apple/

Aha! I think this describes the problem exactly. Disabling fullfsync gets us the performance of using system sqlite. The question is whether it is safe for us to do that.

fanquake commented at 6:36 pm on September 17, 2022: member

Aha! I think this describes the problem exactly. Disabling fullfsync gets us the performance of using system sqlite. The question is whether it is safe for us to do that.

It’s quite possible that Apple is making assumptions about sqlite & the OS, that allows it to make these modifications / other improvements, that we likely cannot. In any case, we should still want this change, (assuming we could come up with a safe patchset for depends (for 25.0)), otherwise by picking brews sqlite, we would then still be choosing the worst of 3 available options, for performance, and continuing to cause confusion / issues downstream.

DrahtBot commented at 4:19 am on September 23, 2022: contributor

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

Conflicts

No conflicts as of last run.

hebasto commented at 8:55 am on September 23, 2022: member

NACK from me.

After having an offline discussion with @fanquake, I agree with his points.

Concept ACK.

Our docs say:

sqlite is required to support for descriptor wallets.

macOS ships with a useable sqlite package, meaning you don’t need to install anything.

But Homebrew can install its sqlite package as an other package’s dependency, and next time during configuring another sqlite library will be used with no user’s intention. And this PR prevents such cases.

Sjors commented at 9:11 am on September 23, 2022: member

There is no reason to assume these are developer environments

I agree with this point. Although most users won’t self compile, I would not assume that most people who self compile are (experienced) developers.

I don’t quite understand why we must avoid the system sqlite lib at all costs, while at the same time we are going to happily load any other number of apple shared libs (which are complete black boxes) at runtime (also true for release binaries).

I also agree with this. If (a rogue) Apple (employee) wanted to steal coins, there are probably more obscure corners of their black box library to put the backdoor in than SQlite.

Is ~3.4s really so bad for importing a bunch of descriptors?

It’s actually worse that this, migrating a legacy wallet to descriptors takes more than ten minutes for me and leaves the wallet in an incorrect state. That said, this PR actually doesn’t fix that problem for me, but it might for others.

I do hope we can fix the real problem soon(tm), it seems @achow101 is optimistic.

jarolrod approved

jarolrod commented at 4:35 am on October 13, 2022: member

ACK d216d714aae36e6f1c95f82aef81a0be74dee2f3

This seems like the best thing to do, assuming whatever apple does with their sqlite can’t introduce any possibility of corrupt wallets or losing coins; which is unlikely considering native builds would have been using apple’s sqlite for some time.

Confirmed macOS 10.14 (currently the minimum supported macOS version) includes a sqlite version greater than 3.7; for reference, it includes SQLite 3.24.0.

PR description is still incorrect in stating that the linked PR had no motivation.

jbesraa commented at 5:49 pm on October 14, 2022: none

I encountered the issue when working with BDK. This fixed the issue for me. Previously it took ~1 minute to complete, now it takes ~3 seconds.

OS: Mac OS Monterey 12.6 Processor: Intel Core i5 SQLite Version 3.37.0

hebasto approved

hebasto commented at 10:59 am on October 17, 2022: member

ACK d216d714aae36e6f1c95f82aef81a0be74dee2f3, I have reviewed the code and it looks OK, I agree it can be merged. No conflicts with our build docs.

Still thinking that “That change was made without any rationale…” in the PR description is incorrect/misleading.

achow101 merged this on Oct 17, 2022

achow101 closed this on Oct 17, 2022

benthecarman commented at 3:19 pm on October 17, 2022: contributor

Would be nice if this is back ported on the effected version

maflcko added the label Needs backport (24.x) on Oct 17, 2022

maflcko removed the label Needs backport (24.x) on Oct 17, 2022

maflcko commented at 3:37 pm on October 17, 2022: member

It can be merged into 24.x as-is (https://github.com/bitcoin/bitcoin/pull/26327), but for the other branches, someone will need to cherry-pick.

fanquake referenced this in commit bb5bcf32ba on Oct 18, 2022

fanquake deleted the branch on Oct 18, 2022

fanquake referenced this in commit 419bdc534f on Oct 18, 2022

fanquake removed the label Needs backport (23.x) on Oct 18, 2022

fanquake commented at 1:38 pm on October 18, 2022: member

Did a cherry-pick for 23.x in #26333.

fanquake referenced this in commit bf42d7de56 on Oct 20, 2022

fanquake removed the label Needs backport (22.x) on Oct 20, 2022

fanquake commented at 6:05 am on October 20, 2022: member

22.x in #26350.

sidhujag referenced this in commit ab43cfcabe on Oct 23, 2022

maflcko referenced this in commit c2d46d7434 on Oct 24, 2022

maflcko referenced this in commit 2d3161fdd5 on Oct 24, 2022

bitcoin locked this on Oct 20, 2023

Revert “build: Use Homebrew’s sqlite package if it is available” #25985

Conflicts