💬 ariard commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2814239023)
> I think it would be valuable to have CTV rather as a taproot op_success.
So I’ve re-read the current version of BIP119 as available in the BIP repository, there is a section about the usage of OP_NOP (`NOP-Default and Recommended Standardness Rules`), however it doesn’t say among the NOP upgradable path and the current upgrade path available with Taproot OP_SUCCESS or an upgrade of the tapscript version.
I’m aware that CTV was designed and drafted for standard, before Taproot got activat
...
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2814239023)
> I think it would be valuable to have CTV rather as a taproot op_success.
So I’ve re-read the current version of BIP119 as available in the BIP repository, there is a section about the usage of OP_NOP (`NOP-Default and Recommended Standardness Rules`), however it doesn’t say among the NOP upgradable path and the current upgrade path available with Taproot OP_SUCCESS or an upgrade of the tapscript version.
I’m aware that CTV was designed and drafted for standard, before Taproot got activat
...
💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050006636)
You could reframe the comment to explain why this is significant:
```
# Wallet created using relative path won't reside in the default wallet dir,
# hence it should not appear in listwalletdir results.
```
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050006636)
You could reframe the comment to explain why this is significant:
```
# Wallet created using relative path won't reside in the default wallet dir,
# hence it should not appear in listwalletdir results.
```
💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050010952)
Can we add more stricter checks on the transaction received here.
For example:
```
tx = wallet.gettransaction(txid)
assert_equal(tx["amount"], 1)
assert_equal(tx["confirmations"], 1)
```
This gives you tighter guarantees and helps with debugging if the test fails.
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050010952)
Can we add more stricter checks on the transaction received here.
For example:
```
tx = wallet.gettransaction(txid)
assert_equal(tx["amount"], 1)
assert_equal(tx["confirmations"], 1)
```
This gives you tighter guarantees and helps with debugging if the test fails.
💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050018648)
We should first validate that file is present at the specified location and accessible and then only try to open and validate other things
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050018648)
We should first validate that file is present at the specified location and accessible and then only try to open and validate other things
💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050015403)
Similar to previous test and all other places, can we fail gracefully with more informative message:
```
assert_equal(..., ..., "Test requires that both wallet paths have the same relative path to the common parent")
```
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050015403)
Similar to previous test and all other places, can we fail gracefully with more informative message:
```
assert_equal(..., ..., "Test requires that both wallet paths have the same relative path to the common parent")
```
💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050007265)
If this assumption ever fails (say due to environment setup differences), the test will break early.
Consider failing gracefully with a more informative message:
```
assert_equal(..., ..., "Relative paths from wallet dirs to common parent must be identical for this test.")
```
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050007265)
If this assumption ever fails (say due to environment setup differences), the test will break early.
Consider failing gracefully with a more informative message:
```
assert_equal(..., ..., "Relative paths from wallet dirs to common parent must be identical for this test.")
```
💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050013797)
Nit: May be a small comment with reason would help here:
```
# No 'failed_watchonly' directory should exist because migration cleanup should be complete
```
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050013797)
Nit: May be a small comment with reason would help here:
```
# No 'failed_watchonly' directory should exist because migration cleanup should be complete
```
💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050019504)
**Minor / Nit:** A separate helper function could be introduced to validate it's BDB for better readability.
Example:
```
def is_bdb_wallet(filepath):
with open(filepath, "rb") as f:
data = f.read(16)
_, _, magic = struct.unpack("QII", data)
return magic == BTREE_MAGIC
```
and then:
```
assert is_bdb_wallet(datfile)
```
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2050019504)
**Minor / Nit:** A separate helper function could be introduced to validate it's BDB for better readability.
Example:
```
def is_bdb_wallet(filepath):
with open(filepath, "rb") as f:
data = f.read(16)
_, _, magic = struct.unpack("QII", data)
return magic == BTREE_MAGIC
```
and then:
```
assert is_bdb_wallet(datfile)
```
💬 shahsb commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2814523446)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2814523446)
Concept ACK
💬 maflcko commented on issue "ci: failure in Windows cross-test":
(https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2814631853)
The error message is a bit confusing, because the benchmarks should be a single process? Thus, it seems odd that another process was accessing the wallet file.
I don't have Windows to debug this, so I guess it is best to temporarily revert. A follow-up can fix the underlying issue and enable the CI check again.
(https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2814631853)
The error message is a bit confusing, because the benchmarks should be a single process? Thus, it seems odd that another process was accessing the wallet file.
I don't have Windows to debug this, so I guess it is best to temporarily revert. A follow-up can fix the underlying issue and enable the CI check again.
📝 maflcko opened a pull request: "ci: Temporarily revert "Drop bench -priority-level in win cross CI""
(https://github.com/bitcoin/bitcoin/pull/32302)
This reverts commit 27f11217ca63e0f8f78f14db139150052dcd9962.
The commit was nice and useful. However, CI doesn't pass, see https://github.com/bitcoin/bitcoin/issues/32291. Temporarily revert it, so that it can be enabled again along with the issue fixed.
(https://github.com/bitcoin/bitcoin/pull/32302)
This reverts commit 27f11217ca63e0f8f78f14db139150052dcd9962.
The commit was nice and useful. However, CI doesn't pass, see https://github.com/bitcoin/bitcoin/issues/32291. Temporarily revert it, so that it can be enabled again along with the issue fixed.
💬 maflcko commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2050124932)
> Before the fix this also fails with the following (built without sanitizers, doesn't fail in Release mode):
That's -ftrapv, added in 2643fa50869f22672cbc72ac497d9c30234075b8
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2050124932)
> Before the fix this also fails with the following (built without sanitizers, doesn't fail in Release mode):
That's -ftrapv, added in 2643fa50869f22672cbc72ac497d9c30234075b8
💬 maflcko commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#issuecomment-2814647839)
lgtm ACK 5cb1241814b9c541c5e5e8daadbbea595f2960ae
(https://github.com/bitcoin/bitcoin/pull/32300#issuecomment-2814647839)
lgtm ACK 5cb1241814b9c541c5e5e8daadbbea595f2960ae
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2814808645)
This or a followup needs to update `doc/dependencies.md` and update the build instructions to change opt-in wording to opt-out. I'll update that here if https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813734342 lands earlier.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2814808645)
This or a followup needs to update `doc/dependencies.md` and update the build instructions to change opt-in wording to opt-out. I'll update that here if https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813734342 lands earlier.
💬 Sjors commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2050262297)
I made a note to add it.
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2050262297)
I made a note to add it.
💬 Sjors commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2814813703)
re-ACK 7f5a35cf4b31f176ff9138d21f3ec677ae7d1bcf
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2814813703)
re-ACK 7f5a35cf4b31f176ff9138d21f3ec677ae7d1bcf
💬 l0rinc commented on pull request "[IBD] prevector: allocate `P2WSH`/`P2TR`/`P2PK` scripts on stack":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2814858797)
A few previous measurements were done on `34` byte prevector, the latest version bumped it to `36` (which included P2PK scripts on stack as well).
I've rerun the `AssumeUTXO`, `reindex-chainstate` and `IBD` benchmarks (both on *SSD* and *HDD*, since they have different performance profiles) and couldn't find a single instance where `master` was any faster.
> this provides an improvement for nodes with default dbcache
@achow101, what other usecases do you think I should measure to make sur
...
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2814858797)
A few previous measurements were done on `34` byte prevector, the latest version bumped it to `36` (which included P2PK scripts on stack as well).
I've rerun the `AssumeUTXO`, `reindex-chainstate` and `IBD` benchmarks (both on *SSD* and *HDD*, since they have different performance profiles) and couldn't find a single instance where `master` was any faster.
> this provides an improvement for nodes with default dbcache
@achow101, what other usecases do you think I should measure to make sur
...
💬 janb84 commented on pull request "doc: Add note for building on macOS (Intel) with CMake ≥ 4.0":
(https://github.com/bitcoin/bitcoin/pull/32289#issuecomment-2814877177)
> > ... that should probably be fixed _properly_, in some way.
>
> I haven't found a way to do it that covers all use cases, is user-friendly, and doesn't introduce excessive complexity.
Maybe one "solution" is not to lean only on homebrew but offer also an alternative package manager e.g. nix ? (nix is still on cmake 3.31 so not sure what will happen when they are on 4.0 )
(https://github.com/bitcoin/bitcoin/pull/32289#issuecomment-2814877177)
> > ... that should probably be fixed _properly_, in some way.
>
> I haven't found a way to do it that covers all use cases, is user-friendly, and doesn't introduce excessive complexity.
Maybe one "solution" is not to lean only on homebrew but offer also an alternative package manager e.g. nix ? (nix is still on cmake 3.31 so not sure what will happen when they are on 4.0 )
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2814926794)
re-ACK 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a
Main changes since my last review:
- commit 3841da0f62fa5e26f679a12c6efbafe1cb17c25f to preserve PSBT GUI coverage
- #32214 landed which made 3841da0f62fa5e26f679a12c6efbafe1cb17c25f and 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a smaller
- #31866 landed which required some some changes in 7dbf06c33508820486eed59aba78d852c6212864
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2814926794)
re-ACK 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a
Main changes since my last review:
- commit 3841da0f62fa5e26f679a12c6efbafe1cb17c25f to preserve PSBT GUI coverage
- #32214 landed which made 3841da0f62fa5e26f679a12c6efbafe1cb17c25f and 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a smaller
- #31866 landed which required some some changes in 7dbf06c33508820486eed59aba78d852c6212864
💬 medobrens83 commented on issue "Bitcoin Kernel Library Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-2814945804)
👍👍🏻💯
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-2814945804)
👍👍🏻💯