Skip to content

Conversation

@fanquake
Copy link
Member

Backport of #25985 to the 23.x branch.

This reverts ee7b84e from bitcoin#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 bitcoin#25724.

Resolves the "build from source" portion of bitcoin#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
bitcoin#25724 (comment),
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.

Github-Pull: bitcoin#25985
Rebased-From: d216d71
@0xB10C
Copy link
Contributor

0xB10C commented Oct 21, 2022

ACK 419bdc5

I've checked that this backports the change from #25985 to 23.x as it says it does.

@dergoegge
Copy link
Member

ACK 419bdc5

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK 419bdc5

I verified that the change matches #25985. I also verified that, on macOS with sqlite3 installed with brew, 23.x uses the homebrew version and with this PR it uses the system version.

When running the below command:

./autogen.sh
./configure --enable-suppress-external-warnings --disable-bench --disable-tests --with-gui=no
cat config.status | grep "sqlite"

On this PR, it outputs:

S["SQLITE_LIBS"]="-lsqlite3"

On 23.x, it outputs:

S["SQLITE_LIBS"]="-L/opt/homebrew/Cellar/sqlite/3.39.4/lib -lsqlite3"
S["SQLITE_CFLAGS"]="-I/opt/homebrew/Cellar/sqlite/3.39.4/include"
S["PKG_CONFIG_PATH"]="/opt/homebrew/opt/qt@5/lib/pkgconfig:/opt/homebrew/opt/sqlite/lib/pkgconfig:"

@stickies-v
Copy link
Contributor

After reviewing #26350, I'm not sure why this backport doesn't do the same docs update? Seems to be like they should be the same?

@maflcko maflcko added this to the 22.1 milestone Oct 21, 2022
@hebasto
Copy link
Member

hebasto commented Oct 21, 2022

@MarcoFalke
image

Did you mean "23.1"?

@maflcko maflcko modified the milestones: 22.1, 23.1 Oct 21, 2022
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 7698366, I have reviewed the code and it looks OK, I agree it can be merged.

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

re-ACK 7698366

@maflcko maflcko merged commit c2d46d7 into bitcoin:23.x Oct 24, 2022
@fanquake fanquake deleted the revert_slow_macos_sqlite_23_x branch October 24, 2022 10:03
delta1 added a commit to delta1/elements that referenced this pull request Sep 24, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants