💬 l0rinc commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1975495352)
to not use all the buffer, where we had problems before.
Do you suggest we use the whole buffer here, like before, or to give it a bigger max value?

💬 l0rinc commented on pull request "Don't zero-after-free `DataStream`: Faster IBD on some configurations":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2690772278)
I have noticed that in debug builds the zeroing is a big part of the IBD flame graphs, but in release mode it's almost completely eliminated.

🤔 maflcko reviewed a pull request: "Require sqlite when building the wallet"
(https://github.com/bitcoin/bitcoin/pull/31961#pullrequestreview-2650908562)
commit bafba0658c33e88664326f0b94a170f55bf7b5bd is wrong 🏃

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

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: commit bafba0658c3
...

💬 maflcko commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1975503951)
now you removed both. Neither 2, nor 0 are correct. It would be good to have 1.

after you push a commit to github, you can review the diff yourself. This should help to spot obvious errors

💬 rkrux commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1975517150)
Should this call be removed as well? Right now, the `wallet_descriptor.py` test will be skipped if the python sqlite3 package is unavailable. I would assume that if the wallet is enabled (for which `skip_if_no_wallet()` 2 lines above is sufficient), I would want this test to run all the time instead of being skipped conditionally.

What do others think?

💬 maflcko commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1975518923)
> to not use all the buffer, where we had problems before.

What problems? A problem was introduced in the base58 fuzz target in the previous change, however you are changing the base32/64 here and below.

Generally, if there is no reason to change the code, I think it shouldn't be changed. Otherwise, it will just be confusing, hard to review, and may even introduce problems.

💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1975520921)
You're right, muscle memory 😅
Thanks!

💬 maflcko commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1975522001)
The python3 sqlite package is different from the sqlite system package. Those are two unrelated checks. The python package may be missing when the other is available.

💬 rkrux commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1975537816)
True.

My point was if the wallet is enabled, I would prefer to have this test run always for which this python package should be installed as well. But I now realise it is a developer's POV.

Not removing this call would be acceptable if we assume not everyone who compile the wallet would want to run the functional tests, which I guess could be a big enough group.

💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1975539845)
> @fjahr @ismaelsadeeq `txfeerate` is now deprecated, but I kept it hidden. IMO it makes sense to have a deprecated rpc call hidden as only users who were using it before should know about it and may need info, they can continue using `help settxfee`

I justified it in another comment below. If you consider that it should not be hidden for some reason or if it is the way it has always been done and you want to keep the form, I will change.

💬 Sjors commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1975542169)
> if we assume not everyone who compile the wallet would want to run the functional tests

Probably the vast majority.

💬 Sjors commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1975544391)
The fuzz binary built on my machine, so I'm confused.

💬 l0rinc commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1975548347)
> What problems? A problem was introduced in the base58 fuzz target in the previous change, however you are changing the base32/64 here and below.

I've learned that using the whole buffer can be problematic for long running tasks, and since base conversion is mostly scale dependent anyway, we'd get better coverage of special values (e.g. all zeros, powers of the base, etc), by using a value that isn't too large.

> there is no reason to change the code

This PR is meant to unify the relat
...

💬 Sjors commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1975551868)
But anyway, I meant to drop only the duplicate. Will push fix.

💬 Sjors commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2690859025)
Fixed https://github.com/bitcoin/bitcoin/pull/31961/commits/ac6cde731314d981391bc313c98d671c68211d33#r1975551868

⚠️ placintaalexandru opened an issue: "Ability to build the library"
(https://github.com/bitcoin/bitcoin/issues/31964)
### Please describe the feature you'd like to see added.

Is it possible t build the library so it can be used by external applications as well?

I would like to be able to build the project, do `make install` and then have the headers and libs in `/usr/local/include` and `/usr/local/lib`

### Is your feature related to a problem, if so please describe it.

_No response_

### Describe the solution you'd like

_No response_

### Describe any alternatives you've considered

_No response_

### Plea
...

💬 pinheadmz commented on issue "Ability to build the library":
(https://github.com/bitcoin/bitcoin/issues/31964#issuecomment-2690876328)
First of all we have migrated to `cmake` instead of `make install` ;-)

Bitcoin Core does not plan on exposing a shared library, this is an old discussion: https://github.com/bitcoin/bitcoin/pull/5084

💬 purpleKarrot commented on issue "Ability to build the library":
(https://github.com/bitcoin/bitcoin/issues/31964#issuecomment-2690911534)
> First of all we have migrated to `cmake` instead of `make install` ;-)

This is not an argument. CMake's "Unix Makefiles" generator does generate an `install` target, as [documented here](https://cmake.org/cmake/help/latest/generator/Unix%20Makefiles.html).

Executing `make install` in the build directory installs executables and manpages, no headers and no libraries.

💬 placintaalexandru commented on issue "Ability to build the library":
(https://github.com/bitcoin/bitcoin/issues/31964#issuecomment-2690914756)
Yes, I saw the transition to cmake. My question is more about the ability to build project as a library so other projects can link against it

💬 purpleKarrot commented on issue "Ability to build the library":
(https://github.com/bitcoin/bitcoin/issues/31964#issuecomment-2690928572)
When `BUILD_KERNEL_LIB` is set to ON, a `libbitcoinkernel.so` is installed together with a `libbitcoinkernel.pc` for pgkconfig. But no header files. Also no CMake config files.