💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182555489)
Can we move this closer to its usage - to obviate that the first check doesn't need this yet.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182555489)
Can we move this closer to its usage - to obviate that the first check doesn't need this yet.
📝 instagibbs opened a pull request: "WIP p2p: Relax BlockRequestAllowed to respond to advertised blocks"
(https://github.com/bitcoin/bitcoin/pull/32869)
First commit adds coverage to a similar case to #13370 . h/t @dergoegge for detecting this issue independently. Documentation of this behavior with a test is then followed by a proposed change where anytime we considered something `VALID_TRANSACTIONS` and potentially advertised via compact blocks, we should respond to requests for the `getdata` if the peer fell back for whatever reason.
If the second commit is deemed controversial it can be shelved and current behavior documented alone.
(https://github.com/bitcoin/bitcoin/pull/32869)
First commit adds coverage to a similar case to #13370 . h/t @dergoegge for detecting this issue independently. Documentation of this behavior with a test is then followed by a proposed change where anytime we considered something `VALID_TRANSACTIONS` and potentially advertised via compact blocks, we should respond to requests for the `getdata` if the peer fell back for whatever reason.
If the second commit is deemed controversial it can be shelved and current behavior documented alone.
💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2182835137)
Also, it doesn't seem to be working right now anyway, see the CI failure (needs rebase)
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2182835137)
Also, it doesn't seem to be working right now anyway, see the CI failure (needs rebase)
💬 dergoegge commented on pull request "WIP p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3032377986)
Concept ACK
> h/t dergoegge for detecting this issue independently.
For context, I found this case (as demonstrated by the first commit in the PR) by adding a netsplit oracle (see https://gist.github.com/dergoegge/3036796551095a9ce7535fb5d74e6656#detecting-network-split-bugs) to [fuzzamoto](https://github.com/dergoegge/fuzzamoto). While extremely unrealistic/unlikely to trigger in the wild, I'd still consider this timeout case a bug, especially because it could cause disconnects across th
...
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3032377986)
Concept ACK
> h/t dergoegge for detecting this issue independently.
For context, I found this case (as demonstrated by the first commit in the PR) by adding a netsplit oracle (see https://gist.github.com/dergoegge/3036796551095a9ce7535fb5d74e6656#detecting-network-split-bugs) to [fuzzamoto](https://github.com/dergoegge/fuzzamoto). While extremely unrealistic/unlikely to trigger in the wild, I'd still consider this timeout case a bug, especially because it could cause disconnects across th
...
💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3032396743)
I split off a non-behavior-changing commit that adds `avoid_script_path` to `FillPSBT`. I think it makes sense to pass this option here, although in general we may want to refactor `FillPSBT` to an options struct instead of this ever expanding argument list. That could be an independent or prerequisite PR.
I then did the same for `SignPSBTInput`, where I also think struct would be better.
Rinse and repeat for `::SignTransaction`.
Will continue later to further split 09334ae0bd7939ed7476
...
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3032396743)
I split off a non-behavior-changing commit that adds `avoid_script_path` to `FillPSBT`. I think it makes sense to pass this option here, although in general we may want to refactor `FillPSBT` to an options struct instead of this ever expanding argument list. That could be an independent or prerequisite PR.
I then did the same for `SignPSBTInput`, where I also think struct would be better.
Rinse and repeat for `::SignTransaction`.
Will continue later to further split 09334ae0bd7939ed7476
...
💬 furszy commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2182893076)
as it is used only here now, could just inline it?
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2182893076)
as it is used only here now, could just inline it?
🤔 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)