💬 jonatack commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1644206979)
If helpful, a similar pros/cons discussion took place in March 2021 starting from https://github.com/bitcoin/bitcoin/pull/21328#issuecomment-789650071.
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1644206979)
If helpful, a similar pros/cons discussion took place in March 2021 starting from https://github.com/bitcoin/bitcoin/pull/21328#issuecomment-789650071.
💬 MarcoFalke commented on pull request "util: Remove DirIsWritable, GetUniquePath":
(https://github.com/bitcoin/bitcoin/pull/28075#issuecomment-1644223545)
rebased
(https://github.com/bitcoin/bitcoin/pull/28075#issuecomment-1644223545)
rebased
💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644240564)
Can someone compile with `-save-temps` and get me the sha256_sse4.s file?
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644240564)
Can someone compile with `-save-temps` and get me the sha256_sse4.s file?
👍 MarcoFalke approved a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1539670029)
lgtm. Left two nits, feel free to ignore.
lgtm ACK 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d 🚆
<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+krxU1A3Yux4bpwZNLvVBKy0wL
...
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1539670029)
lgtm. Left two nits, feel free to ignore.
lgtm ACK 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d 🚆
<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+krxU1A3Yux4bpwZNLvVBKy0wL
...
💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269708017)
Missing self-include?
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269708017)
Missing self-include?
💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269712862)
it may be nice to include univalue.h and then mark it as `export` for iwyu. This would allow include-sites to drop the univalue.h include, since it should be obvious that there is a univalue dependency with the `univalue_helpers.h` include already.
Also, this shouldn't increase compile cost, because it would be rare (in this case) for a call-site to include this header, but not need the univalue header.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269712862)
it may be nice to include univalue.h and then mark it as `export` for iwyu. This would allow include-sites to drop the univalue.h include, since it should be obvious that there is a univalue dependency with the `univalue_helpers.h` include already.
Also, this shouldn't increase compile cost, because it would be rare (in this case) for a call-site to include this header, but not need the univalue header.
💬 mzumsande commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1644266248)
I didn't get to address the suggestions by @jonatack and @MarcoFalke in time - they make sense to me and could be done in a separate PR that reworks the test a bit deeper than just fixing the timeout - just noting that I'm probably not going to get to this, at least for the next couple of weeks.
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1644266248)
I didn't get to address the suggestions by @jonatack and @MarcoFalke in time - they make sense to me and could be done in a separate PR that reworks the test a bit deeper than just fixing the timeout - just noting that I'm probably not going to get to this, at least for the next couple of weeks.
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269737484)
So I just tried this and you don't need to generate *any* additional blocks. A single call to `self.generateblock(self.nodes[1], output=f"raw(61{'ff'*998000})", transactions=[])` is enough.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269737484)
So I just tried this and you don't need to generate *any* additional blocks. A single call to `self.generateblock(self.nodes[1], output=f"raw(61{'ff'*998000})", transactions=[])` is enough.
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269738696)
yeah, ive got this too, will push in a few minutes
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269738696)
yeah, ive got this too, will push in a few minutes
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269740640)
Any reason for this node?
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269740640)
Any reason for this node?
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269743318)
My understanding is that inlining should be fine here, because if the previous nonroot code was removed, the call would fail with "path not found", does it not?
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269743318)
My understanding is that inlining should be fine here, because if the previous nonroot code was removed, the call would fail with "path not found", does it not?
💬 MarcoFalke commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1644292192)
Sure:
```
2023-07-20T16:40:23Z [main] [init/common.cpp:153] [LogPackageVersion] Bitcoin Core version v25.99.0-d23fda05842b (release build)
2023-07-20T16:40:56Z [qt-rpcconsole] [wallet/wallet.h:895] [WalletLogPrintf] [default wallet] Migrating wallet storage database from BerkeleyDB to SQLite.
2023-07-20T17:10:25Z [scheduler] [net.cpp:1523] [DumpAddresses] [net] Flushed 0 addresses to peers.dat 56ms
bitcoin-qt: wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wa
...
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1644292192)
Sure:
```
2023-07-20T16:40:23Z [main] [init/common.cpp:153] [LogPackageVersion] Bitcoin Core version v25.99.0-d23fda05842b (release build)
2023-07-20T16:40:56Z [qt-rpcconsole] [wallet/wallet.h:895] [WalletLogPrintf] [default wallet] Migrating wallet storage database from BerkeleyDB to SQLite.
2023-07-20T17:10:25Z [scheduler] [net.cpp:1523] [DumpAddresses] [net] Flushed 0 addresses to peers.dat 56ms
bitcoin-qt: wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wa
...
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269749581)
nope leftover from last iteration sorry, removing
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269749581)
nope leftover from last iteration sorry, removing
💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269745657)
I don't really find this fn to be a `UniValue` helper, pretty much all the logic is sighash specific. While thinking for a better place to sit, I ended up thinking the better solution may be to keep it where it is but just remove the (imo unnecessary) dependency on UV? Offloading the default value and type check to a helper function like `ParseSighashString` is opaque, and even though my approach here is slightly more verbose in the callsite I find the clarity of what's happening to be worth it
...
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269745657)
I don't really find this fn to be a `UniValue` helper, pretty much all the logic is sighash specific. While thinking for a better place to sit, I ended up thinking the better solution may be to keep it where it is but just remove the (imo unnecessary) dependency on UV? Offloading the default value and type check to a helper function like `ParseSighashString` is opaque, and even though my approach here is slightly more verbose in the callsite I find the clarity of what's happening to be worth it
...
💬 hebasto commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644335078)
> Can someone compile with `-save-temps` and get me the sha256_sse4.s file?
For the master branch:
- https://github.com/hebasto/artefacts/blob/master/issue-28111/asm-master/sha256_sse4.s
Master branch + the [patch](https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644167053):
- https://github.com/hebasto/artefacts/blob/master/issue-28111/asm-patched/sha256_sse4.s
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644335078)
> Can someone compile with `-save-temps` and get me the sha256_sse4.s file?
For the master branch:
- https://github.com/hebasto/artefacts/blob/master/issue-28111/asm-master/sha256_sse4.s
Master branch + the [patch](https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644167053):
- https://github.com/hebasto/artefacts/blob/master/issue-28111/asm-patched/sha256_sse4.s
💬 achow101 commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1269784399)
In ecdee0c3f802c6308c358a5ee501d2e031c8dcf2 "miniscript: make GetStackSize independent of P2WSH context"
The comment above this function is outdated.
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1269784399)
In ecdee0c3f802c6308c358a5ee501d2e031c8dcf2 "miniscript: make GetStackSize independent of P2WSH context"
The comment above this function is outdated.
💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1269792567)
"RPC is for testing only" was used since this is a hidden RPC.
> I think it would also be nice if documentation gave a little hint of what the new and tried are tables so the functionality is discoverable.
liked the ChatGPT documentation! will update the PR to use some of this.
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1269792567)
"RPC is for testing only" was used since this is a hidden RPC.
> I think it would also be nice if documentation gave a little hint of what the new and tried are tables so the functionality is discoverable.
liked the ChatGPT documentation! will update the PR to use some of this.
💬 theuni commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1644357788)
Makes sense to me. It seems a bit premature maybe, but I agree with the goal of not carrying this type of baggage over to CMake.
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1644357788)
Makes sense to me. It seems a bit premature maybe, but I agree with the goal of not carrying this type of baggage over to CMake.
👍 MarcoFalke approved a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1539812584)
lgtm ACK 068b523ee7720e6a392efc294ff89bf936a3e2ac 🍈
<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 068b523ee7720e6
...
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1539812584)
lgtm ACK 068b523ee7720e6a392efc294ff89bf936a3e2ac 🍈
<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 068b523ee7720e6
...
💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644379013)
Ok so what seems to be happening is that in your build, `%16` gets mapped to `(%rsp)` (aka the memory address stored in the stack pointer), which in a few places gets used as e.g. `8+%16`, which results in `8+(%rsp)`, and that appears to be invalid asm code.
On my system (and presumably ~every build we have since this code was introduced), `%16` is mapped to `16(%rsp)` (16 bytes further than the memory address stored in the stack pointer), and thus `8+%16` becomes `8+16(%rsp)`, which is 24 by
...
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644379013)
Ok so what seems to be happening is that in your build, `%16` gets mapped to `(%rsp)` (aka the memory address stored in the stack pointer), which in a few places gets used as e.g. `8+%16`, which results in `8+(%rsp)`, and that appears to be invalid asm code.
On my system (and presumably ~every build we have since this code was introduced), `%16` is mapped to `16(%rsp)` (16 bytes further than the memory address stored in the stack pointer), and thus `8+%16` becomes `8+16(%rsp)`, which is 24 by
...