💬 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.
💬 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**?