💬 maflcko commented on pull request "test: check proper OP_2ROT behavior":
(https://github.com/bitcoin/bitcoin/pull/33047#issuecomment-3112272988)
lgtm ACK b94c6356a29b03def6337c91caabb3b8642187e8
(https://github.com/bitcoin/bitcoin/pull/33047#issuecomment-3112272988)
lgtm ACK b94c6356a29b03def6337c91caabb3b8642187e8
💬 maflcko commented on pull request "doc: update headers and remove manual TOCs":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3112293137)
lgtm ACK ca38cf701dc635d39db987105c5b2ccc87fd9815 🌔
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK ca38cf701dc635d3
...
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3112293137)
lgtm ACK ca38cf701dc635d39db987105c5b2ccc87fd9815 🌔
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK ca38cf701dc635d3
...
💬 ArmchairCryptologist commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3112315080)
> feerate estimation currently doesn’t do below 1 s/vB, so this PR might be more involved than it seems at first glance
IMHO it would probably be reasonable to stagger changes to wallet/fee estimation for one release, and only change the thresholds for mempool/relay and possibly mining initially. Based on some stats from my own nodes, I would assume that relaying sub-1 sat/vB transactions will still be spotty for some time after a new version with changed defaults is released.
Node 1: Of 1
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3112315080)
> feerate estimation currently doesn’t do below 1 s/vB, so this PR might be more involved than it seems at first glance
IMHO it would probably be reasonable to stagger changes to wallet/fee estimation for one release, and only change the thresholds for mempool/relay and possibly mining initially. Based on some stats from my own nodes, I would assume that relaying sub-1 sat/vB transactions will still be spotty for some time after a new version with changed defaults is released.
Node 1: Of 1
...
💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r2227646845)
It seems so. Will fix release note if I have to retouch.
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r2227646845)
It seems so. Will fix release note if I have to retouch.
💬 maflcko commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2227753763)
I don't understand this change. The commit message just says "util: Add value_type and span constructor to (W)Txid", but it doesn't say why or where this is needed. Compiling without this (locally) seems to pass.
Also, conceptually, it seems to go in the opposite direction that we want to go in. This makes implicit construction from any byte span to `transaction_identifier` possible again. The constructor `transaction_identifier(const uint256& wrapped)` is private and will still catch those cas
...
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2227753763)
I don't understand this change. The commit message just says "util: Add value_type and span constructor to (W)Txid", but it doesn't say why or where this is needed. Compiling without this (locally) seems to pass.
Also, conceptually, it seems to go in the opposite direction that we want to go in. This makes implicit construction from any byte span to `transaction_identifier` possible again. The constructor `transaction_identifier(const uint256& wrapped)` is private and will still catch those cas
...
💬 maflcko commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2227638927)
encapsualtes -> encapsulates [spelling error in comment “encapsualtes” makes the word misspelled]
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2227638927)
encapsualtes -> encapsulates [spelling error in comment “encapsualtes” makes the word misspelled]
💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3112459168)
> > > @HowHsu, are you going to include the test or a patch to reproduce the issue? I’m happy to ACK it either way.
> >
> >
> > Hi furszy, I thought the test I put already demonstrate the issue, though it is not proper to be included to the codebase.
>
> Are you referring to the one you linked in [#32878 (comment)](https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3045485948)? That link returns a 404.
Sorry for that, check this one: https://github.com/HowHsu/bitcoin/commit/e9
...
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3112459168)
> > > @HowHsu, are you going to include the test or a patch to reproduce the issue? I’m happy to ACK it either way.
> >
> >
> > Hi furszy, I thought the test I put already demonstrate the issue, though it is not proper to be included to the codebase.
>
> Are you referring to the one you linked in [#32878 (comment)](https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3045485948)? That link returns a 404.
Sorry for that, check this one: https://github.com/HowHsu/bitcoin/commit/e9
...
💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2227792656)
> We still need to write the minversion record for newly created wallets so that they can be opened in older versions with the expected feature set.
I think I may be missing something, but I don't understand what feature set this is referring to. `CanSupportFeature()` is only called on legacy BDB wallets, but they can't open sqlite descriptor wallets anyway, so retaining the code here for compatibility does not seem needed.
It is fine to keep it for consistency or for documentation purposes, b
...
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2227792656)
> We still need to write the minversion record for newly created wallets so that they can be opened in older versions with the expected feature set.
I think I may be missing something, but I don't understand what feature set this is referring to. `CanSupportFeature()` is only called on legacy BDB wallets, but they can't open sqlite descriptor wallets anyway, so retaining the code here for compatibility does not seem needed.
It is fine to keep it for consistency or for documentation purposes, b
...
💬 maflcko commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2227804494)
unconfiremd -> unconfirmed [spelling error in “unconfiremd parents”]
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2227804494)
unconfiremd -> unconfirmed [spelling error in “unconfiremd parents”]
💬 maflcko commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2227809802)
in the first commit (after the rebase): This is now required. Otherwise:
```
assert_equal(testres[0]["reject-reason"], "missing-inputs")
^^^^^^^^^^^^
NameError: name 'assert_equal' is not defined
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2227809802)
in the first commit (after the rebase): This is now required. Otherwise:
```
assert_equal(testres[0]["reject-reason"], "missing-inputs")
^^^^^^^^^^^^
NameError: name 'assert_equal' is not defined
💬 maflcko commented on pull request "[29.x] Backport #32521":
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3112537332)
the ci failure is unrelated. The fix was https://github.com/bitcoin/bitcoin/commit/cd8089c20baaaee329cbf7951195953a9f86d7cf, but that never made it into 29.x
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3112537332)
the ci failure is unrelated. The fix was https://github.com/bitcoin/bitcoin/commit/cd8089c20baaaee329cbf7951195953a9f86d7cf, but that never made it into 29.x
💬 maflcko commented on pull request "test: Add functional tests for blockreconstructionextratxn parameter":
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3112587616)
Could turn into draft while CI is failing?
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3112587616)
Could turn into draft while CI is failing?
🤔 janb84 reviewed a pull request: "doc: update headers and remove manual TOCs"
(https://github.com/bitcoin/bitcoin/pull/33040#pullrequestreview-3050853475)
ACK ca38cf701dc635d39db987105c5b2ccc87fd9815
changes since last ACK:
- Removed manual TOCs
(https://github.com/bitcoin/bitcoin/pull/33040#pullrequestreview-3050853475)
ACK ca38cf701dc635d39db987105c5b2ccc87fd9815
changes since last ACK:
- Removed manual TOCs
📝 fanquake converted_to_draft a pull request: "test: Add functional tests for blockreconstructionextratxn parameter"
(https://github.com/bitcoin/bitcoin/pull/33023)
This adds tests for the -blockreconstructionextratxn parameter which controls the extra transaction pool used for compact block reconstruction.
Uses policy rejected transactions to populate the extra pool for tests.
(https://github.com/bitcoin/bitcoin/pull/33023)
This adds tests for the -blockreconstructionextratxn parameter which controls the extra transaction pool used for compact block reconstruction.
Uses policy rejected transactions to populate the extra pool for tests.
🤔 yuvicc reviewed a pull request: "doc: update headers and remove manual TOCs"
(https://github.com/bitcoin/bitcoin/pull/33040#pullrequestreview-3051001175)
re-ACK ca38cf701dc635d39db987105c5b2ccc87fd9815
(https://github.com/bitcoin/bitcoin/pull/33040#pullrequestreview-3051001175)
re-ACK ca38cf701dc635d39db987105c5b2ccc87fd9815
👍 fanquake approved a pull request: "doc: Fix typos in asmap README"
(https://github.com/bitcoin/bitcoin/pull/33049#pullrequestreview-3051009188)
ACK b59dc21847d3bb20c0d77af5b4ca0ae5d8e56221
(https://github.com/bitcoin/bitcoin/pull/33049#pullrequestreview-3051009188)
ACK b59dc21847d3bb20c0d77af5b4ca0ae5d8e56221
🚀 fanquake merged a pull request: "doc: Fix typos in asmap README"
(https://github.com/bitcoin/bitcoin/pull/33049)
(https://github.com/bitcoin/bitcoin/pull/33049)
💬 fanquake commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2228108478)
In 9481c77420845316e825855ac589aa8a10bff057: Should drop the redundant `\n` on all lines.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2228108478)
In 9481c77420845316e825855ac589aa8a10bff057: Should drop the redundant `\n` on all lines.
💬 fanquake commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2228114963)
In 74932c62f9627a2eee634b1dd16fe2582c258a74: Seems a bit awkaward to add an (undocumented) special case here for asmap? Can't we just fail if the value given isn't a valid path, or stop supporting this behaviour? cc @ryanofsky
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2228114963)
In 74932c62f9627a2eee634b1dd16fe2582c258a74: Seems a bit awkaward to add an (undocumented) special case here for asmap? Can't we just fail if the value given isn't a valid path, or stop supporting this behaviour? cc @ryanofsky
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-3112940862)
Updated 1f96d1cefc721b579b7aa3b05c2e65df62c0c5cf -> 10c3a0689a3b73eb2b291ee81dd85066fa32497d ([spendblock_8](https://github.com/TheCharlatan/bitcoin/tree/spendblock_8) -> [spendblock_9](https://github.com/TheCharlatan/bitcoin/tree/spendblock_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_8..spendblock_9))
* Fixed silent merge conflict with #32521
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-3112940862)
Updated 1f96d1cefc721b579b7aa3b05c2e65df62c0c5cf -> 10c3a0689a3b73eb2b291ee81dd85066fa32497d ([spendblock_8](https://github.com/TheCharlatan/bitcoin/tree/spendblock_8) -> [spendblock_9](https://github.com/TheCharlatan/bitcoin/tree/spendblock_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_8..spendblock_9))
* Fixed silent merge conflict with #32521