💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182696708)
I've added `.github/ci-test-each-commit-exec.py` & `.github/workflows/ci.yml`. I'll add a commit for Guix once the build passes.
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182696708)
I've added `.github/ci-test-each-commit-exec.py` & `.github/workflows/ci.yml`. I'll add a commit for Guix once the build passes.
💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3032145576)
The `walletprocesspsbt` RPC now also has this option. Added test coverage by expanding `wallet_taproot.py`.
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3032145576)
The `walletprocesspsbt` RPC now also has this option. Added test coverage by expanding `wallet_taproot.py`.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2182726209)
Unfortunate that it can only be resolved by marking the function as `NOLINT`. Addressed.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2182726209)
Unfortunate that it can only be resolved by marking the function as `NOLINT`. Addressed.
💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2182756040)
Just for additional reference, the check is already sometimes failing on current master: https://github.com/bitcoin/bitcoin/issues/30797#issue-2503043392
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2182756040)
Just for additional reference, the check is already sometimes failing on current master: https://github.com/bitcoin/bitcoin/issues/30797#issue-2503043392
💬 hebasto commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3032234676)
Rebased.
@janb84
> Any hints how to verify / test this ?
>
> Have tried to see any difference between this PR and master but it's not clear to me what difference I should see. When used on #32697 there is a difference that without this PR `build/test/functional/data/util/*` is populated with this PR the util directory is not being created (looking at the source this is logical but unsure if there needs to be things done after 32697 is merged or not having the directory is intentional )
...
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3032234676)
Rebased.
@janb84
> Any hints how to verify / test this ?
>
> Have tried to see any difference between this PR and master but it's not clear to me what difference I should see. When used on #32697 there is a difference that without this PR `build/test/functional/data/util/*` is populated with this PR the util directory is not being created (looking at the source this is logical but unsure if there needs to be things done after 32697 is merged or not having the directory is intentional )
...
💬 pinheadmz commented on issue "intermittent issue in wallet_keypool.py: assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0) AssertionError: not(1725366101 == 0)":
(https://github.com/bitcoin/bitcoin/issues/30797#issuecomment-3032235787)
Oh ha! I should've checked closed issues before opening #32793. I didn't see any other recent failures outside my own PR so I suspected it was just me.
(https://github.com/bitcoin/bitcoin/issues/30797#issuecomment-3032235787)
Oh ha! I should've checked closed issues before opening #32793. I didn't see any other recent failures outside my own PR so I suspected it was just me.
💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182775275)
Added a commit for Guix as well.
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182775275)
Added a commit for Guix as well.
👋 fanquake's pull request is ready for review: "cmake: Use `AUTHOR_WARNING` for warnings"
(https://github.com/bitcoin/bitcoin/pull/32865)
(https://github.com/bitcoin/bitcoin/pull/32865)
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182562681)
Nit: this seems a bit repetitive now that the above lines were edited - if we keep it, consider unifying how we write `scriptPubKey`:
```suggestion
// This means sigops in the spent scriptPubKey count toward the limit.
```
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182562681)
Nit: this seems a bit repetitive now that the above lines were edited - if we keep it, consider unifying how we write `scriptPubKey`:
```suggestion
// This means sigops in the spent scriptPubKey count toward the limit.
```
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182560754)
```suggestion
// `fAccurate` means correctly accounting sigops for CHECKMULTISIG(VERIFY)s with 16 pubkeys or fewer. This
```
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182560754)
```suggestion
// `fAccurate` means correctly accounting sigops for CHECKMULTISIG(VERIFY)s with 16 pubkeys or fewer. This
```
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182812849)
Wanted to understand the orders of magnitude we're talking about - especially for low-end raspberry pi 4 devices -, ran a `-reindex-chainstate` with `-assumevalid=0` until 400k blocks to see the correlation between script size and validation time - and the order of magnitude of validating tens of thousands of scripts in a single tx (since these are normally skipped during my IBD benchmarks at these historic heights).
I simply subtracted the subsequent times to get the real time (including bl
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182812849)
Wanted to understand the orders of magnitude we're talking about - especially for low-end raspberry pi 4 devices -, ran a `-reindex-chainstate` with `-assumevalid=0` until 400k blocks to see the correlation between script size and validation time - and the order of magnitude of validating tens of thousands of scripts in a single tx (since these are normally skipped during my IBD benchmarks at these historic heights).
I simply subtracted the subsequent times to get the real time (including bl
...
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182817167)
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2182817167)
💬 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.