💬 pinheadmz commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1533568647)
Still waiting for CI to finish but I removed the need for internet access by looking up the name `"localhost"` instead. That is a non-numeric lookup that `getaddrinfo()` can do without the actual DNS. I think I also fixed the ipv6 test issue.
I'm interested in following the worker pool idea to make these requests async as opposed to new threads.
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1533568647)
Still waiting for CI to finish but I removed the need for internet access by looking up the name `"localhost"` instead. That is a non-numeric lookup that `getaddrinfo()` can do without the actual DNS. I think I also fixed the ipv6 test issue.
I'm interested in following the worker pool idea to make these requests async as opposed to new threads.
💬 brunoerg commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#issuecomment-1533573510)
@josibake thank you, I just updated the description.
> so why not make the RPC expose the full functionality? I think I'd be more annoyed if I saw that DisconnectNode supported subnet in the code, but the RPC wouldn't let me use it.
> I think disconnecting from all ports on the same IP seems like a compelling enough use case for a very small change.
Yea, I only opened this one because the change would be really small, and since `DisconnectNode` supports subnet, it might worth.
(https://github.com/bitcoin/bitcoin/pull/26576#issuecomment-1533573510)
@josibake thank you, I just updated the description.
> so why not make the RPC expose the full functionality? I think I'd be more annoyed if I saw that DisconnectNode supported subnet in the code, but the RPC wouldn't let me use it.
> I think disconnecting from all ports on the same IP seems like a compelling enough use case for a very small change.
Yea, I only opened this one because the change would be really small, and since `DisconnectNode` supports subnet, it might worth.
💬 hebasto commented on pull request "build: Add CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1533601248)
Updated on top of the recent https://github.com/hebasto/bitcoin/pull/15.
CI [build](https://cirrus-ci.com/build/5395835672854528) is :green_circle:
Guix builds:
```
040eb5ee40cf57dcfe34caf8598cd856ac8585565c74386905feccbeb1868830 guix-build-7cc0a620fd74/output/aarch64-linux-gnu/SHA256SUMS.part
bb542a379b8c8ff54c645b4260ddf26265efcd0fc204fa09ce5eaf201f30ddb7 guix-build-7cc0a620fd74/output/aarch64-linux-gnu/bitcoin-7cc0a620fd74-aarch64-linux-gnu-debug.tar.gz
c8a307a90343c8553ff98e1720
...
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1533601248)
Updated on top of the recent https://github.com/hebasto/bitcoin/pull/15.
CI [build](https://cirrus-ci.com/build/5395835672854528) is :green_circle:
Guix builds:
```
040eb5ee40cf57dcfe34caf8598cd856ac8585565c74386905feccbeb1868830 guix-build-7cc0a620fd74/output/aarch64-linux-gnu/SHA256SUMS.part
bb542a379b8c8ff54c645b4260ddf26265efcd0fc204fa09ce5eaf201f30ddb7 guix-build-7cc0a620fd74/output/aarch64-linux-gnu/bitcoin-7cc0a620fd74-aarch64-linux-gnu-debug.tar.gz
c8a307a90343c8553ff98e1720
...
💬 pinheadmz commented on issue "Add RPC to set transaction comment":
(https://github.com/bitcoin/bitcoin/issues/12594#issuecomment-1533608333)
@GSPP `txid` by definition is a unique identifier for a transaction. Comments can be added to new transactions in some RPCs like `sendtoaddress` and those comments are visible in the GUI. What do you need? The ability to edit a transaction comment in the GUI?
(https://github.com/bitcoin/bitcoin/issues/12594#issuecomment-1533608333)
@GSPP `txid` by definition is a unique identifier for a transaction. Comments can be added to new transactions in some RPCs like `sendtoaddress` and those comments are visible in the GUI. What do you need? The ability to edit a transaction comment in the GUI?
👍 furszy approved a pull request: "Introduce `MockableDatabase` for wallet unit tests"
(https://github.com/bitcoin/bitcoin/pull/26715#pullrequestreview-1411697889)
ACK 33e2b82
(https://github.com/bitcoin/bitcoin/pull/26715#pullrequestreview-1411697889)
ACK 33e2b82
💬 furszy commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1181279440)
tiny nit:
just because I'm using this in another PR:
```c++
change_pos == -1 ? std::nullopt : std::make_optional(change_pos)
```
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1181279440)
tiny nit:
just because I'm using this in another PR:
```c++
change_pos == -1 ? std::nullopt : std::make_optional(change_pos)
```
💬 hebasto commented on pull request "qt: 25.0rc2 translations update":
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1533660835)
Updated.
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1533660835)
Updated.
🤔 achow101 reviewed a pull request: "Wallet: estimate the size of signed inputs using descriptors"
(https://github.com/bitcoin/bitcoin/pull/26567#pullrequestreview-1411720143)
The `MultiSigningProvider` seems like a fine approach.
(https://github.com/bitcoin/bitcoin/pull/26567#pullrequestreview-1411720143)
The `MultiSigningProvider` seems like a fine approach.
💬 achow101 commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1184204984)
In 9e8df2de1750b823492c18a924fefbd7d474819e "descriptor: introduce a method to get the satisfaction size"
Is `MultiADescriptor` supposed to be missing a `MaxSatisfactionWeight`?
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1184204984)
In 9e8df2de1750b823492c18a924fefbd7d474819e "descriptor: introduce a method to get the satisfaction size"
Is `MultiADescriptor` supposed to be missing a `MaxSatisfactionWeight`?
⚠️ geenutts opened an issue: "Github"
(https://github.com/bitcoin/bitcoin/issues/27568)

(https://github.com/bitcoin/bitcoin/issues/27568)

✅ geenutts closed an issue: "Github"
(https://github.com/bitcoin/bitcoin/issues/27568)
(https://github.com/bitcoin/bitcoin/issues/27568)
💬 mzumsande commented on issue "Coinstats index corrupted after invalidateblock and clean shutdown":
(https://github.com/bitcoin/bitcoin/issues/27558#issuecomment-1533742759)
A more detailed explanation: The indexes have a `Rewind()` function to reverse the effects of blocks (which calls `CustomRewind` for the coinstats index). However, once the index is synced, `Rewind` will only be invoked through `BlockConnected()` validationinterface signals, not when blocks are disconnected. So when `invalidateblock` is called, the state of the coinstatsindex will still point to the old block (unless a new block is connected before the node is stopped).
After restarting the n
...
(https://github.com/bitcoin/bitcoin/issues/27558#issuecomment-1533742759)
A more detailed explanation: The indexes have a `Rewind()` function to reverse the effects of blocks (which calls `CustomRewind` for the coinstats index). However, once the index is synced, `Rewind` will only be invoked through `BlockConnected()` validationinterface signals, not when blocks are disconnected. So when `invalidateblock` is called, the state of the coinstatsindex will still point to the old block (unless a new block is connected before the node is stopped).
After restarting the n
...
💬 achow101 commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1533778952)
ACK 9bf2d303295594b8327ce0d67b9c1de98701f80a
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1533778952)
ACK 9bf2d303295594b8327ce0d67b9c1de98701f80a
🚀 achow101 merged a pull request: "prune, import: allow pruning to work during loadblock import"
(https://github.com/bitcoin/bitcoin/pull/24957)
(https://github.com/bitcoin/bitcoin/pull/24957)
💬 theStack commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1184370245)
tiny nit: should keep the list alphabetically sorted (that is, add the new RPC between "getpeerinfo" and "getrawmempool")
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1184370245)
tiny nit: should keep the list alphabetically sorted (that is, add the new RPC between "getpeerinfo" and "getrawmempool")
🤔 theStack reviewed a pull request: "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1412001811)
Concept ACK
Was a bit surprised to see that the fee-delta is returned in BTC as unit rather than Satoshis, I assume our RPC guidelines (saying _"**Always** use `AmountFromValue` and `ValueFromAmount` when inputting or outputting monetary values"_) have higher priority over the idea to simply use the same unit used for `prioritisetransaction`? Probably miners would be thankful to not need multiply the result by 100000000 first in order to delete mapDelta entries (and it would also make the tes
...
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1412001811)
Concept ACK
Was a bit surprised to see that the fee-delta is returned in BTC as unit rather than Satoshis, I assume our RPC guidelines (saying _"**Always** use `AmountFromValue` and `ValueFromAmount` when inputting or outputting monetary values"_) have higher priority over the idea to simply use the same unit used for `prioritisetransaction`? Probably miners would be thankful to not need multiply the result by 100000000 first in order to delete mapDelta entries (and it would also make the tes
...
💬 theStack commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1184372400)
nit, could get rid of extra variable:
```suggestion
for (const auto& [txid, delta_exists_pair] : mempool.GetPrioritisationMap()) {
```
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1184372400)
nit, could get rid of extra variable:
```suggestion
for (const auto& [txid, delta_exists_pair] : mempool.GetPrioritisationMap()) {
```
💬 theStack commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1184382264)
nit: `COIN` is defined in messages.py and should be imported from there
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1184382264)
nit: `COIN` is defined in messages.py and should be imported from there
👍 jarolrod approved a pull request: "qt: 25.0rc2 translations update"
(https://github.com/bitcoin/bitcoin/pull/27517#pullrequestreview-1412184995)
ACK 20c076d0567da56637e69a26a8cc4e7d99124ebd
Made sure we're adhering to the translation process and ran the script. I sanity tested loading different translations within the gui to make sure nothing is wildly off.
(https://github.com/bitcoin/bitcoin/pull/27517#pullrequestreview-1412184995)
ACK 20c076d0567da56637e69a26a8cc4e7d99124ebd
Made sure we're adhering to the translation process and ran the script. I sanity tested loading different translations within the gui to make sure nothing is wildly off.