🤔 janb84 reviewed a pull request: "cmake: Create subdirectories in build tree in advance"
(https://github.com/bitcoin/bitcoin/pull/32773#pullrequestreview-2983389789)
ACK 0b7d038972315b5537cc42038094902ebd5dd8cf
This PR changes :
- Recursive Glob -> 'explicit' non recursive GLOB
- Subdirectories now are created in advance, so that there aren't any problems with missing paths for sym-linking.
The PR fixes a "bug" and communicates the intentions of the code better.
Can reproduce the successful subfolder creation and therefor file sym-linking :
<details>
Master (fails see `invalid_signer.py` ):
```shell
$ ls -l
total 4
-rwxr-xr-x 1 jan
...
(https://github.com/bitcoin/bitcoin/pull/32773#pullrequestreview-2983389789)
ACK 0b7d038972315b5537cc42038094902ebd5dd8cf
This PR changes :
- Recursive Glob -> 'explicit' non recursive GLOB
- Subdirectories now are created in advance, so that there aren't any problems with missing paths for sym-linking.
The PR fixes a "bug" and communicates the intentions of the code better.
Can reproduce the successful subfolder creation and therefor file sym-linking :
<details>
Master (fails see `invalid_signer.py` ):
```shell
$ ls -l
total 4
-rwxr-xr-x 1 jan
...
🤔 furszy reviewed a pull request: "rpc: use CScheduler for relocking wallet and remove RPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32862#pullrequestreview-2983397697)
ACK 8a1765795fd3bff79d790102ca7cefa8fd9b204c
(https://github.com/bitcoin/bitcoin/pull/32862#pullrequestreview-2983397697)
ACK 8a1765795fd3bff79d790102ca7cefa8fd9b204c
💬 ismaelsadeeq commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032501112)
Still NACK
fwiw what I meant in my suggestion here https://github.com/bitcoin/bitcoin/pull/32615#discussion_r2107593916
is what @maflcko recently commented.
It's been a while since I take a look at this, I think it's best to discuss exact suggestion in the tracking issue.
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032501112)
Still NACK
fwiw what I meant in my suggestion here https://github.com/bitcoin/bitcoin/pull/32615#discussion_r2107593916
is what @maflcko recently commented.
It's been a while since I take a look at this, I think it's best to discuss exact suggestion in the tracking issue.
💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3032587480)
Guix Build:
```bash
7c9bae09c11b66251bcab7abe66d3446da64ca2386fbc67066c1f8194df778b9 guix-build-04c4cc059e44/output/aarch64-linux-gnu/SHA256SUMS.part
ffb0670da219af40ce2c986e33b8446bbc6d60ebfa31fd142f91a22a50a3ba89 guix-build-04c4cc059e44/output/aarch64-linux-gnu/bitcoin-04c4cc059e44-aarch64-linux-gnu-debug.tar.gz
1b54fb59042b8abc7d2b7079de3f9b16770f6434c41049aebf4e0bbb4278596b guix-build-04c4cc059e44/output/aarch64-linux-gnu/bitcoin-04c4cc059e44-aarch64-linux-gnu.tar.gz
fc14d2a51e6a1b12
...
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3032587480)
Guix Build:
```bash
7c9bae09c11b66251bcab7abe66d3446da64ca2386fbc67066c1f8194df778b9 guix-build-04c4cc059e44/output/aarch64-linux-gnu/SHA256SUMS.part
ffb0670da219af40ce2c986e33b8446bbc6d60ebfa31fd142f91a22a50a3ba89 guix-build-04c4cc059e44/output/aarch64-linux-gnu/bitcoin-04c4cc059e44-aarch64-linux-gnu-debug.tar.gz
1b54fb59042b8abc7d2b7079de3f9b16770f6434c41049aebf4e0bbb4278596b guix-build-04c4cc059e44/output/aarch64-linux-gnu/bitcoin-04c4cc059e44-aarch64-linux-gnu.tar.gz
fc14d2a51e6a1b12
...
💬 tnndbtc commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032596320)
@ismaelsadeeq Not sure your comment of "Still NACK" is to this PR, or to other people's comments. From your suggestion: `I believe the correct fix is to update the test to generate and mine the correct number of transactions with a fee rate that will always reliably return a fee rate.`
This PR is to use a hard-coded fee list which is generated from an actual good random seed that reliably return the fee rate. In my understanding, this matches your suggestion. If not, please clarify. Thanks.
...
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032596320)
@ismaelsadeeq Not sure your comment of "Still NACK" is to this PR, or to other people's comments. From your suggestion: `I believe the correct fix is to update the test to generate and mine the correct number of transactions with a fee rate that will always reliably return a fee rate.`
This PR is to use a hard-coded fee list which is generated from an actual good random seed that reliably return the fee rate. In my understanding, this matches your suggestion. If not, please clarify. Thanks.
...
💬 ismaelsadeeq commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3032599431)
reACK 705791cd and tested it again by building with release config.
```terminal
-- Install configuration: "Release"
-- Installing: /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/install/bin/bitcoin-wallet
-- Installing: /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/install/share/man/man1/bitcoin-wallet.1
-- Installing: /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/install/bin/bitcoin
-- Installing: /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/install/bin
...
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3032599431)
reACK 705791cd and tested it again by building with release config.
```terminal
-- Install configuration: "Release"
-- Installing: /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/install/bin/bitcoin-wallet
-- Installing: /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/install/share/man/man1/bitcoin-wallet.1
-- Installing: /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/install/bin/bitcoin
-- Installing: /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/install/bin
...
💬 tnndbtc commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032604420)
> Just ensure enough random transactions have been created to reliably return a fee estimate in any run?
@maflcko Random is random. How can we ensure randomly generated transactions can reliably return a fee estimate **in any run**?
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032604420)
> Just ensure enough random transactions have been created to reliably return a fee estimate in any run?
@maflcko Random is random. How can we ensure randomly generated transactions can reliably return a fee estimate **in any run**?
🤔 rkrux reviewed a pull request: "rpc: add optional nodeid param to filter getpeerinfo"
(https://github.com/bitcoin/bitcoin/pull/32741#pullrequestreview-2983518394)
Concept ACK
Useful addition, I can see myself using the filtering here.
A release note would be helpful as per this: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes
(https://github.com/bitcoin/bitcoin/pull/32741#pullrequestreview-2983518394)
Concept ACK
Useful addition, I can see myself using the filtering here.
A release note would be helpful as per this: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes
💬 rkrux commented on pull request "rpc: add optional nodeid param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2183003505)
Can mention in the RPC help.
```diff
- "Returns data about each connected network peer as a json array of objects.",
+ "Returns data about each connected network peer as a json array of objects.\n"
+ "Optionally filter by peer index.\n",
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2183003505)
Can mention in the RPC help.
```diff
- "Returns data about each connected network peer as a json array of objects.",
+ "Returns data about each connected network peer as a json array of objects.\n"
+ "Optionally filter by peer index.\n",
✅ maflcko closed a pull request: "fee estimate test: fix #31944 by handling a legitimate scenario that …"
(https://github.com/bitcoin/bitcoin/pull/32615)
(https://github.com/bitcoin/bitcoin/pull/32615)
💬 maflcko commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032629888)
I am not sure if you are using an LLM in the background, but the goal of a pull request is not to open it and then ask others to provide the solution for you, then ask others to review it.
The burden to understand the issue, to propose the fix, and to understand the fix is on the author.
Closing for now, because this is clearly the wrong approach here.
Discussion can continue, of course. Also, if a proper solution is found, a new pull request can be opened.
> Random is random.
...
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032629888)
I am not sure if you are using an LLM in the background, but the goal of a pull request is not to open it and then ask others to provide the solution for you, then ask others to review it.
The burden to understand the issue, to propose the fix, and to understand the fix is on the author.
Closing for now, because this is clearly the wrong approach here.
Discussion can continue, of course. Also, if a proper solution is found, a new pull request can be opened.
> Random is random.
...
💬 ismaelsadeeq commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032663440)
> @ismaelsadeeq I'm not sure if your comment "Still NACK" is directed at this PR.
Yes, as it stands, I don't think this PR is reviewable, as other reviewers have also mentioned.
> This PR proposes using a hard-coded fee list generated from a properly seeded random source that reliably returns the fee rate. From my understanding, this matches your suggestion. If not, please clarify. Thanks.
@tnndbtc I think you may have misread the comment. Randomness should have defined bounds. What
...
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032663440)
> @ismaelsadeeq I'm not sure if your comment "Still NACK" is directed at this PR.
Yes, as it stands, I don't think this PR is reviewable, as other reviewers have also mentioned.
> This PR proposes using a hard-coded fee list generated from a properly seeded random source that reliably returns the fee rate. From my understanding, this matches your suggestion. If not, please clarify. Thanks.
@tnndbtc I think you may have misread the comment. Randomness should have defined bounds. What
...
💬 theStack commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3032665076)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3032665076)
Concept ACK
🚀 fanquake merged a pull request: "doc: Add workaround for vcpkg issue with paths with embedded spaces"
(https://github.com/bitcoin/bitcoin/pull/32858)
(https://github.com/bitcoin/bitcoin/pull/32858)
💬 tnndbtc commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032722076)
> I am not sure if you are using an LLM in the background, but the goal of a pull request is not to open it and then ask others to provide the solution for you, then ask others to review it.
>
> The burden to understand the issue, to propose the fix, and to understand the fix is on the author.
>
> Closing for now, because this is clearly the wrong approach here.
>
> Discussion can continue, of course. Also, if a proper solution is found, a new pull request can be opened.
>
> > Random
...
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3032722076)
> I am not sure if you are using an LLM in the background, but the goal of a pull request is not to open it and then ask others to provide the solution for you, then ask others to review it.
>
> The burden to understand the issue, to propose the fix, and to understand the fix is on the author.
>
> Closing for now, because this is clearly the wrong approach here.
>
> Discussion can continue, of course. Also, if a proper solution is found, a new pull request can be opened.
>
> > Random
...
⚠️ maflcko opened an issue: "fuzz: Speed up mini_miner fuzz target?"
(https://github.com/bitcoin/bitcoin/issues/32870)
Looks like it is somewhat slow (c.f. https://cirrus-ci.com/task/6602192649453568?logs=ci#L5401), so it would be nice to make it faster.
Possibly with txgraph, see https://github.com/bitcoin-core/qa-assets/pull/227#issuecomment-3032434420
(https://github.com/bitcoin/bitcoin/issues/32870)
Looks like it is somewhat slow (c.f. https://cirrus-ci.com/task/6602192649453568?logs=ci#L5401), so it would be nice to make it faster.
Possibly with txgraph, see https://github.com/bitcoin-core/qa-assets/pull/227#issuecomment-3032434420
👍 hebasto approved a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32811#pullrequestreview-2983722824)
ACK 44b07b2d5a6937b71c984b4c2d3edb382346f96b.
(https://github.com/bitcoin/bitcoin/pull/32811#pullrequestreview-2983722824)
ACK 44b07b2d5a6937b71c984b4c2d3edb382346f96b.
💬 tnndbtc commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3032777163)
@maflcko Is there a way to retrieve debug.log while the test fails so it may provide more clues on why RPC took longer?
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3032777163)
@maflcko Is there a way to retrieve debug.log while the test fails so it may provide more clues on why RPC took longer?
💬 purpleKarrot commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3032802799)
ACK 04c4cc059e44a6d1b4f520af10c649648a002ebc
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3032802799)
ACK 04c4cc059e44a6d1b4f520af10c649648a002ebc
🤔 ryanofsky reviewed a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-2983599593)
re: https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3029455047
Sorry I didn't post my complete suggestion before. Here is the change I would suggest based on the current PR (2e8a21a43a3c321c9f3a8c75024d036165658225):
<details><summary>diff</summary>
<p>
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4253,30 +4253,16 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// Make a backup of the DB in the wallet's d
...
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-2983599593)
re: https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3029455047
Sorry I didn't post my complete suggestion before. Here is the change I would suggest based on the current PR (2e8a21a43a3c321c9f3a8c75024d036165658225):
<details><summary>diff</summary>
<p>
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4253,30 +4253,16 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// Make a backup of the DB in the wallet's d
...
💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183052507)
In commit "test: Migration of a wallet ending in `/`" (499099320b138c3d895d3ad8a622a08e404008d6)
This fails on my system because the test is racy. By the time backup is created time.time() is higher than the backup time.
Also the os.path.exists() return value is discarded here so this doesn't actually check anything.
Here are the changes I made to fix these problems. (Another approach might be to set a mock time instead)
<details><summary>diff</summary>
<p>
```diff
--- a/test/fu
...
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2183052507)
In commit "test: Migration of a wallet ending in `/`" (499099320b138c3d895d3ad8a622a08e404008d6)
This fails on my system because the test is racy. By the time backup is created time.time() is higher than the backup time.
Also the os.path.exists() return value is discarded here so this doesn't actually check anything.
Here are the changes I made to fix these problems. (Another approach might be to set a mock time instead)
<details><summary>diff</summary>
<p>
```diff
--- a/test/fu
...