💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2081959909)
I opened a separate issue for this here https://github.com/bitcoin/bitcoin/issues/32462#issue-3052551984
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2081959909)
I opened a separate issue for this here https://github.com/bitcoin/bitcoin/issues/32462#issue-3052551984
💬 l0rinc commented on pull request "bench: replace benchmark block with more representative one (413567 → 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2081962158)
Understandable, but that's why I added the hashes here, to make it self-validating.
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2081962158)
Understandable, but that's why I added the hashes here, to make it self-validating.
🚀 fanquake merged a pull request: "refactor: Removals after bdb removal"
(https://github.com/bitcoin/bitcoin/pull/32438)
(https://github.com/bitcoin/bitcoin/pull/32438)
💬 hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867062653)
> > It is documented, and it is pre-defined by CI itself:
>
> It's not in our docs, or in our CI code, which means our CI would "work", because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a `.cpp` file.
What are you suggesting should be changed to improve or avoid this situation?
> Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If
...
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867062653)
> > It is documented, and it is pre-defined by CI itself:
>
> It's not in our docs, or in our CI code, which means our CI would "work", because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a `.cpp` file.
What are you suggesting should be changed to improve or avoid this situation?
> Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If
...
💬 fanquake commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867064817)
> This does not apply to the "raw" GitHub Actions workflows, where this code takes effect.
Can't someone just compile and run the same binaries locally?
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867064817)
> This does not apply to the "raw" GitHub Actions workflows, where this code takes effect.
Can't someone just compile and run the same binaries locally?
💬 hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867073689)
> > This does not apply to the "raw" GitHub Actions workflows, where this code takes effect.
>
> Can't someone just compile and run the same binaries locally?
Certainly, they can. But displaying a message box isn't an issue when running binaries locally, so there's no need to alter the environment in that case.
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867073689)
> > This does not apply to the "raw" GitHub Actions workflows, where this code takes effect.
>
> Can't someone just compile and run the same binaries locally?
Certainly, they can. But displaying a message box isn't an issue when running binaries locally, so there's no need to alter the environment in that case.
🤔 pablomartin4btc reviewed a pull request: "wallet: init, don't error out when loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2828866727)
Concept ACK
You could have a few/ several legacy wallets on the `settings.json`. Perhaps list them all together on a warning or just warn that there is at least one (on the GUI you would get a pop up on each otherwise).
(https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2828866727)
Concept ACK
You could have a few/ several legacy wallets on the `settings.json`. Perhaps list them all together on a warning or just warn that there is at least one (on the GUI you would get a pop up on each otherwise).
💬 pablomartin4btc commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2081981059)
_<ins>nit</ins>_: it's not a failure on a load, it's not allowed/ possible anymore... just a suggestion, could be another text message... (also checking a flag to send the warning only once... or with the list of all of them if many)
```suggestion
warnings.push_back(strprintf(_("There are legacy wallets in settings.json which are no longer supported.\n\nPlease migrate to a descriptor wallet using the migration tool (migratewallet RPC or the GUI option)."), walletFile));
```
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2081981059)
_<ins>nit</ins>_: it's not a failure on a load, it's not allowed/ possible anymore... just a suggestion, could be another text message... (also checking a flag to send the warning only once... or with the list of all of them if many)
```suggestion
warnings.push_back(strprintf(_("There are legacy wallets in settings.json which are no longer supported.\n\nPlease migrate to a descriptor wallet using the migration tool (migratewallet RPC or the GUI option)."), walletFile));
```
💬 hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867088113)
As an alternative, we could explicitly set our own `SUPPRESS_ABORT_MESSAGE_BOX` environment variable in the workflow, and gate the code with this variable instead, removing the `#if defined(_MSC_VER)` condition.
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867088113)
As an alternative, we could explicitly set our own `SUPPRESS_ABORT_MESSAGE_BOX` environment variable in the workflow, and gate the code with this variable instead, removing the `#if defined(_MSC_VER)` condition.
💬 laanwj commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2867090172)
> From what I can tell, compilers on Linux systems, will be defining _GNU_SOURCE, which results in glibc defining _POSIX_C_SOURCE to 200809L; so undefining it, and setting it to an earlier value does not seem like the correct behaviour for us, or even required
Yes, there is never a valid reason to set it to a lower value. The idea of the macro is to tell the header files "i know about POSIX version X and want to use it".
Usually this is defined by the build system, or at most at the top of
...
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2867090172)
> From what I can tell, compilers on Linux systems, will be defining _GNU_SOURCE, which results in glibc defining _POSIX_C_SOURCE to 200809L; so undefining it, and setting it to an earlier value does not seem like the correct behaviour for us, or even required
Yes, there is never a valid reason to set it to a lower value. The idea of the macro is to tell the header files "i know about POSIX version X and want to use it".
Usually this is defined by the build system, or at most at the top of
...
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2082064876)
Fixed
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2082064876)
Fixed
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2082066284)
Taken, thanks
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2082066284)
Taken, thanks
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2867201958)
Forced pushed to address recent comments see [c8a3fabe..31e3808d](https://github.com/bitcoin/bitcoin/compare/c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6..31e3808df9c59d36a07cad07df89ae1bf9e63000)
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2867201958)
Forced pushed to address recent comments see [c8a3fabe..31e3808d](https://github.com/bitcoin/bitcoin/compare/c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6..31e3808df9c59d36a07cad07df89ae1bf9e63000)
💬 laanwj commented on pull request "bench: replace benchmark block with more representative one (413567 → 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2082115539)
Hahaha agree it would be extremely far-fetched to put data in a specific block, just to add it in the repository two years later.
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2082115539)
Hahaha agree it would be extremely far-fetched to put data in a specific block, just to add it in the repository two years later.
📝 ismaelsadeeq opened a pull request: "test: fix an incorrect `feature_fee_estimation.py` subtest"
(https://github.com/bitcoin/bitcoin/pull/32463)
Attempt to fix #32461
In the `estimatesmartfee` RPC, we return the maximum of the following: the feerate estimate for the target, `minrelaytxfee`, and `mempoolminfee`.
https://github.com/bitcoin/bitcoin/blob/9a05b45da60d214cb1e5a50c3d2293b1defc9bb0/src/rpc/fees.cpp#L85
The test `test_feerate_mempoolminfee`, originally introduced in https://github.com/bitcoin/bitcoin/commit/ea31caf6b4c182c6f10f136548f84e603800511c, is incorrect.
It should append the higher value used to start the node
...
(https://github.com/bitcoin/bitcoin/pull/32463)
Attempt to fix #32461
In the `estimatesmartfee` RPC, we return the maximum of the following: the feerate estimate for the target, `minrelaytxfee`, and `mempoolminfee`.
https://github.com/bitcoin/bitcoin/blob/9a05b45da60d214cb1e5a50c3d2293b1defc9bb0/src/rpc/fees.cpp#L85
The test `test_feerate_mempoolminfee`, originally introduced in https://github.com/bitcoin/bitcoin/commit/ea31caf6b4c182c6f10f136548f84e603800511c, is incorrect.
It should append the higher value used to start the node
...
💬 ismaelsadeeq commented on issue "test: `acceptstalefeeestimates` failure in `feature_fee_estimation` after duplicate coinbase tx weight reservation fix":
(https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2867564112)
@l0rinc Thanks for the thorough report.
However, the failure is not in the `acceptstalefeeestimates` subtest, but in `test_feerate_mempoolminfee`.
I believe the issue is that we are not appending `high_val` to `self.fees_per_kb` before checking the estimates.
I attempted to fix this in #32463.
---
Also, it's quite strange that the test started failing on this commit https://github.com/bitcoin/bitcoin/commit/6b165f5906fc53bd10bedff85a6ef26e0aabdc5c
I haven't looked into that yet.
(https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2867564112)
@l0rinc Thanks for the thorough report.
However, the failure is not in the `acceptstalefeeestimates` subtest, but in `test_feerate_mempoolminfee`.
I believe the issue is that we are not appending `high_val` to `self.fees_per_kb` before checking the estimates.
I attempted to fix this in #32463.
---
Also, it's quite strange that the test started failing on this commit https://github.com/bitcoin/bitcoin/commit/6b165f5906fc53bd10bedff85a6ef26e0aabdc5c
I haven't looked into that yet.
💬 achow101 commented on pull request "test: Remove legacy wallet RPC overloads":
(https://github.com/bitcoin/bitcoin/pull/32452#issuecomment-2867645207)
Fixed the linter and reorganized as suggested.
(https://github.com/bitcoin/bitcoin/pull/32452#issuecomment-2867645207)
Fixed the linter and reorganized as suggested.
💬 l0rinc commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-2867652250)
Thanks for fixing it so quickly, I can confirm that this fixes the test for the failing `--random=3450808900320758527`.
Someone more knowledgeable in this area of the code should also look at at, but from my part it's a lightweight ACK.
ACK 2c5f26bc6ac47e3a6a8555e9a6c60832322a36e8
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-2867652250)
Thanks for fixing it so quickly, I can confirm that this fixes the test for the failing `--random=3450808900320758527`.
Someone more knowledgeable in this area of the code should also look at at, but from my part it's a lightweight ACK.
ACK 2c5f26bc6ac47e3a6a8555e9a6c60832322a36e8
👍 TheCharlatan approved a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2829494150)
ACK ab878a7e741073574336c9c4b1d41c6cf80b0d6a
Guix build (aarch64):
```
2d1c74e19e10c55a45e137f151f77f0e0628c96f0664949ba730c249e8065597 guix-build-ab878a7e7410/output/aarch64-linux-gnu/SHA256SUMS.part
26cb4ed3dcaf1bfc2d2f9cdc34e8648df11e2aed33bdeb8618a6d8a3370ba0e3 guix-build-ab878a7e7410/output/aarch64-linux-gnu/bitcoin-ab878a7e7410-aarch64-linux-gnu-debug.tar.gz
756e79f7a151b67edc7b607f4b3b0baa72025dbf7cc158e27485f885fbd6adc1 guix-build-ab878a7e7410/output/aarch64-linux-gnu/bitcoin-a
...
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2829494150)
ACK ab878a7e741073574336c9c4b1d41c6cf80b0d6a
Guix build (aarch64):
```
2d1c74e19e10c55a45e137f151f77f0e0628c96f0664949ba730c249e8065597 guix-build-ab878a7e7410/output/aarch64-linux-gnu/SHA256SUMS.part
26cb4ed3dcaf1bfc2d2f9cdc34e8648df11e2aed33bdeb8618a6d8a3370ba0e3 guix-build-ab878a7e7410/output/aarch64-linux-gnu/bitcoin-ab878a7e7410-aarch64-linux-gnu-debug.tar.gz
756e79f7a151b67edc7b607f4b3b0baa72025dbf7cc158e27485f885fbd6adc1 guix-build-ab878a7e7410/output/aarch64-linux-gnu/bitcoin-a
...
💬 JeremyRubin commented on pull request "[Policy] Discourage Unsigned Annexes":
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2867708417)
squashed the CI fixmes to get "test each commit" to pass.
@Sjors no one understands the annex, so you're not alone. You can see https://groups.google.com/g/bitcoindev/c/Q5j2Kb6XeHI?pli=1 for some context on efforts to "standardize" the annex currently ongoing.
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2867708417)
squashed the CI fixmes to get "test each commit" to pass.
@Sjors no one understands the annex, so you're not alone. You can see https://groups.google.com/g/bitcoindev/c/Q5j2Kb6XeHI?pli=1 for some context on efforts to "standardize" the annex currently ongoing.