💬 arejula27 commented on pull request "test: check `scanning` field from `getwalletinfo`":
(https://github.com/bitcoin/bitcoin/pull/31768#issuecomment-2637800811)
ACK `bb0879d`
The test looks good as it improves the verification of an RPC call field. However, the verified field produces different values depending on the scan situation:
> "scanning" : { (json object) current scanning details, or false if no scan is in progress>
> "duration" : n, (numeric) elapsed seconds since scan start
> "progress" : n (numeric) scanning pro
...
(https://github.com/bitcoin/bitcoin/pull/31768#issuecomment-2637800811)
ACK `bb0879d`
The test looks good as it improves the verification of an RPC call field. However, the verified field produces different values depending on the scan situation:
> "scanning" : { (json object) current scanning details, or false if no scan is in progress>
> "duration" : n, (numeric) elapsed seconds since scan start
> "progress" : n (numeric) scanning pro
...
🤔 murchandamus reviewed a pull request: "feefrac: add support for evaluating at given size"
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2596789900)
Concept ACK, I also really like the idea of improving CFeeRate.
ACK up to 5147b5d602d1f55d7c9f497cf0cfc23263f5caf5, still mulling over the last three commits.
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2596789900)
Concept ACK, I also really like the idea of improving CFeeRate.
ACK up to 5147b5d602d1f55d7c9f497cf0cfc23263f5caf5, still mulling over the last three commits.
✅ maflcko closed a pull request: "contrib: fix BUILDDIR in gen-bitcoin-conf script"
(https://github.com/bitcoin/bitcoin/pull/31332)
(https://github.com/bitcoin/bitcoin/pull/31332)
💬 maflcko commented on pull request "contrib: fix BUILDDIR in gen-bitcoin-conf script":
(https://github.com/bitcoin/bitcoin/pull/31332#issuecomment-2637843152)
Ok, closing for https://github.com/bitcoin/bitcoin/pull/31742
(https://github.com/bitcoin/bitcoin/pull/31332#issuecomment-2637843152)
Ok, closing for https://github.com/bitcoin/bitcoin/pull/31742
📝 theuni opened a pull request: "kernel: Avoid duplicating symbols in the kernel and downstream users"
(https://github.com/bitcoin/bitcoin/pull/31807)
The possibility of this happening came up in the discussion here: https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2635223309, and it turned that we already had a case of it in our code.
tl;dr: Certain symbols when defined in headers may end up existing in both the shared lib and the application using it, leading to subtle bugs. More info can be found [here](https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg503669.html).
The solution is generally to avoid defining static
...
(https://github.com/bitcoin/bitcoin/pull/31807)
The possibility of this happening came up in the discussion here: https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2635223309, and it turned that we already had a case of it in our code.
tl;dr: Certain symbols when defined in headers may end up existing in both the shared lib and the application using it, leading to subtle bugs. More info can be found [here](https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg503669.html).
The solution is generally to avoid defining static
...
✅ maflcko closed an issue: "bitcoind crash"
(https://github.com/bitcoin/bitcoin/issues/31712)
(https://github.com/bitcoin/bitcoin/issues/31712)
💬 maflcko commented on issue "bitcoind crash":
(https://github.com/bitcoin/bitcoin/issues/31712#issuecomment-2637856878)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.
(https://github.com/bitcoin/bitcoin/issues/31712#issuecomment-2637856878)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.
💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637878793)
> Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).
CI fails: https://github.com/theuni/bitcoin/actions/runs/13163570761/job/36738155052
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637878793)
> Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).
CI fails: https://github.com/theuni/bitcoin/actions/runs/13163570761/job/36738155052
💬 theuni commented on pull request "kernel: Avoid duplicating symbols in the kernel and downstream users":
(https://github.com/bitcoin/bitcoin/pull/31807#issuecomment-2637909939)
Thinking about this some more, this would mostly only be a problem if we attempted to define a native c++ api for the kernel, as opposed to the [C api](https://github.com/bitcoin/bitcoin/pull/30595) which does not expose our internal headers. Because currently, we mark all symbols as externally visible.
But still, with a c++17-style `inline` variable in a header, it's not clear to me that all linkers are guaranteed to do the right thing in all cases (I admit I might be totally wrong on that),
...
(https://github.com/bitcoin/bitcoin/pull/31807#issuecomment-2637909939)
Thinking about this some more, this would mostly only be a problem if we attempted to define a native c++ api for the kernel, as opposed to the [C api](https://github.com/bitcoin/bitcoin/pull/30595) which does not expose our internal headers. Because currently, we mark all symbols as externally visible.
But still, with a c++17-style `inline` variable in a header, it's not clear to me that all linkers are guaranteed to do the right thing in all cases (I admit I might be totally wrong on that),
...
💬 ismaelsadeeq commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2637909965)
@murchandamus
> Concept ACK, I also really like the idea of improving CFeeRate.
You will also like to https://github.com/bitcoin/bitcoin/pull/31363/commits/46ffaf98503c7f6c8a8d9b9a188f541d77e08b1a
> If this aligns with your idea, I can create a PR for it although the test currently assumes `CFeeRate` represents a fee rate per kb rather than a fraction, so I had to touch the test a bit.
I think after this and #31363 making `CFeeRate` a wrapper around `FeePerVSize` will be a good
...
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2637909965)
@murchandamus
> Concept ACK, I also really like the idea of improving CFeeRate.
You will also like to https://github.com/bitcoin/bitcoin/pull/31363/commits/46ffaf98503c7f6c8a8d9b9a188f541d77e08b1a
> If this aligns with your idea, I can create a PR for it although the test currently assumes `CFeeRate` represents a fee rate per kb rather than a fraction, so I had to touch the test a bit.
I think after this and #31363 making `CFeeRate` a wrapper around `FeePerVSize` will be a good
...
💬 maflcko commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2637928003)
Reminds me that the CI script will silently continue after an error, see https://cirrus-ci.com/task/4909301485010944?logs=ci#L194. It would be good to stop using bash for scripts, or force pipefail (etc) to be set in all of them.
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2637928003)
Reminds me that the CI script will silently continue after an error, see https://cirrus-ci.com/task/4909301485010944?logs=ci#L194. It would be good to stop using bash for scripts, or force pipefail (etc) to be set in all of them.
💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637937419)
> > Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).
>
> CI fails: https://github.com/theuni/bitcoin/actions/runs/13163570761/job/36738155052
Yeah, the above is only enough to test `bitcoin-chainstate.exe`. In reality, the condition in the header would need to be something like:
`#elif defined(MSC_VER) && !defined(STATIC_LIBBITCOINKERNEL) && !defined(BITCOIN_IN
...
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637937419)
> > Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).
>
> CI fails: https://github.com/theuni/bitcoin/actions/runs/13163570761/job/36738155052
Yeah, the above is only enough to test `bitcoin-chainstate.exe`. In reality, the condition in the header would need to be something like:
`#elif defined(MSC_VER) && !defined(STATIC_LIBBITCOINKERNEL) && !defined(BITCOIN_IN
...
💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2637944040)
> * (should also check `-reindex-chainstate` sees them too?)
I tried to add a functional test for this but our test framework assumes an RPC interface is always available, and this error would happen before it is. This would make the test code really clunky, plus `verifychain` tests the same already and `feature_unsupported_utxo_db.py` implicitly tests this (not setting `-testactivationheight=segwit@2` with `-rescan-chainstate` would trigger a ConnectBlock error).
> I think it would be bet
...
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2637944040)
> * (should also check `-reindex-chainstate` sees them too?)
I tried to add a functional test for this but our test framework assumes an RPC interface is always available, and this error would happen before it is. This would make the test code really clunky, plus `verifychain` tests the same already and `feature_unsupported_utxo_db.py` implicitly tests this (not setting `-testactivationheight=segwit@2` with `-rescan-chainstate` would trigger a ConnectBlock error).
> I think it would be bet
...
💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2637986190)
> the reason should be clearly communicated to reviewers and documented for users.
Thanks for the comments. Updated the PR description with all the changes and idea proposed during the revisions.
Also added a release note.
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2637986190)
> the reason should be clearly communicated to reviewers and documented for users.
Thanks for the comments. Updated the PR description with all the changes and idea proposed during the revisions.
Also added a release note.
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r1943642068)
Good point - fixed, and also cleared `blocks` in the end!
While working /testing this branch I directly did picked a value from blockman.m_block_index and introduced `blocks` right before pushing because picking from a `std::unordered_map` mad runs non-deterministic.
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r1943642068)
Good point - fixed, and also cleared `blocks` in the end!
While working /testing this branch I directly did picked a value from blockman.m_block_index and introduced `blocks` right before pushing because picking from a `std::unordered_map` mad runs non-deterministic.
🤔 murchandamus reviewed a pull request: "wallet, rpc: deprecate settxfee"
(https://github.com/bitcoin/bitcoin/pull/31278#pullrequestreview-2596962476)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31278#pullrequestreview-2596962476)
Concept ACK
💬 murchandamus commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1943666577)
Why does `deprecatedrpc=settxfee` need to be specified twice here? Especially on a node without a wallet, the setting doesn’t seem to make sense.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1943666577)
Why does `deprecatedrpc=settxfee` need to be specified twice here? Especially on a node without a wallet, the setting doesn’t seem to make sense.
💬 murchandamus commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1943655940)
The last line here mostly repeats the previous sentence. Perhaps this could be deduplicated.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1943655940)
The last line here mostly repeats the previous sentence. Perhaps this could be deduplicated.
💬 murchandamus commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1943662983)
Unfortunately, there are feerates titled "…fee" all over the codebase. If you are touching the prospective lines already, you could fix the terminology, but if we are going to remove `settxfee` anyway, it’s not necessary to change it first and then remove it.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1943662983)
Unfortunately, there are feerates titled "…fee" all over the codebase. If you are touching the prospective lines already, you could fix the terminology, but if we are going to remove `settxfee` anyway, it’s not necessary to change it first and then remove it.
💬 davidgumberg commented on pull request "depends: Avoid using the `-ffile-prefix-map` compiler option":
(https://github.com/bitcoin/bitcoin/pull/31800#issuecomment-2638037382)
Tested ACK https://github.com/bitcoin/bitcoin/pull/31800/commits/407062f2ac93624f350e9e8a4f641c882a2aaf2f.
The oss-fuzz coverage build failure described in #31770 that I am able to reproduce locally does not happen on this branch.
<details> <summary> Steps to reproduce </summary>
```bash
# setup
git clone --depth 1 https://github.com/google/oss-fuzz.git
git clone https://github.com/bitcoin/bitcoin oss-fuzz/bitcoin
git clone --depth 1 https://github.com/bitcoin-core/qa-assets oss
...
(https://github.com/bitcoin/bitcoin/pull/31800#issuecomment-2638037382)
Tested ACK https://github.com/bitcoin/bitcoin/pull/31800/commits/407062f2ac93624f350e9e8a4f641c882a2aaf2f.
The oss-fuzz coverage build failure described in #31770 that I am able to reproduce locally does not happen on this branch.
<details> <summary> Steps to reproduce </summary>
```bash
# setup
git clone --depth 1 https://github.com/google/oss-fuzz.git
git clone https://github.com/bitcoin/bitcoin oss-fuzz/bitcoin
git clone --depth 1 https://github.com/bitcoin-core/qa-assets oss
...