🤔 jonatack reviewed a pull request: "wallet: Give deprecation warning when loading a legacy wallet"
(https://github.com/bitcoin/bitcoin/pull/27869#pullrequestreview-1514938806)
re-ACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd
modulo https://github.com/bitcoin/bitcoin/pull/27869#pullrequestreview-1477293036
Tested on the command line and with the GUI
<img width="577" alt="Screenshot 2023-07-05 at 10 40 10" src="https://github.com/bitcoin/bitcoin/assets/2415484/042eb69e-9ec7-4e7e-ac19-e072ae977f82">
(https://github.com/bitcoin/bitcoin/pull/27869#pullrequestreview-1514938806)
re-ACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd
modulo https://github.com/bitcoin/bitcoin/pull/27869#pullrequestreview-1477293036
Tested on the command line and with the GUI
<img width="577" alt="Screenshot 2023-07-05 at 10 40 10" src="https://github.com/bitcoin/bitcoin/assets/2415484/042eb69e-9ec7-4e7e-ac19-e072ae977f82">
💬 jonatack commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#discussion_r1253378453)
nit, the similar warning at the end of `CreateWallet()` could be updated to use this expanded one
(https://github.com/bitcoin/bitcoin/pull/27869#discussion_r1253378453)
nit, the similar warning at the end of `CreateWallet()` could be updated to use this expanded one
💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253398027)
yes thanks
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253398027)
yes thanks
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253401345)
Hmm, I think the maximum possible value is currently `101 * MAX_STANDARD_TX_WEIGHT` (after that, `LimitOrphans()` would kick in), so an `unsigned int` should suffice in theory? Maybe use a `uint64_t` if this was to be made configurable by users (see comment above, but not sure if that makes sense).
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1253401345)
Hmm, I think the maximum possible value is currently `101 * MAX_STANDARD_TX_WEIGHT` (after that, `LimitOrphans()` would kick in), so an `unsigned int` should suffice in theory? Maybe use a `uint64_t` if this was to be made configurable by users (see comment above, but not sure if that makes sense).
💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253422959)
Good question, I'm just letting it drop out of scope at the end of the test, but [looking into it more](https://docs.python.org/3/library/threading.html#threading.Thread.daemon) it may actually be staying alive until the entire test runner exits.
Should I add a manual close function?
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253422959)
Good question, I'm just letting it drop out of scope at the end of the test, but [looking into it more](https://docs.python.org/3/library/threading.html#threading.Thread.daemon) it may actually be staying alive until the entire test runner exits.
Should I add a manual close function?
💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253426245)
fantastic, thanks for the patch
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253426245)
fantastic, thanks for the patch
💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253422291)
In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"
nit: I'd prefer if variables were not renamed if they don't need to be, it makes review of this commit slightly harder and unnecessarily breaks git blame. `new_total_fee` is still a valid description of what this variable contains.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253422291)
In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"
nit: I'd prefer if variables were not renamed if they don't need to be, it makes review of this commit slightly harder and unnecessarily breaks git blame. `new_total_fee` is still a valid description of what this variable contains.
💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253424884)
In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"
`GetBumpFeeDiscount` is only used here. It doesn't seem like it's useful to require the caller to calculate the final bumpfee rather than having `SelectionResult` just do that when the the discount is applied and have a single method to return the bumpfees required for a particular selection.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253424884)
In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"
`GetBumpFeeDiscount` is only used here. It doesn't seem like it's useful to require the caller to calculate the final bumpfee rather than having `SelectionResult` just do that when the the discount is applied and have a single method to return the bumpfees required for a particular selection.
💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253427785)
In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"
nit: this comment seems to be misplaced?
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253427785)
In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"
nit: this comment seems to be misplaced?
👍 brunoerg approved a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1515040415)
reACK 7286efece4442c5afe4e19fa6a7d2420444c904c
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1515040415)
reACK 7286efece4442c5afe4e19fa6a7d2420444c904c
💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253443914)
oh sweet! awesome, i'll remove the socks5.py commit and apply this instead
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253443914)
oh sweet! awesome, i'll remove the socks5.py commit and apply this instead
💬 sipa commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1622227095)
@ajtowns Yes, I don't think we have any requirement or even strong desire to go beyond ancestor-set-based linearization quality (= sufficient for single child pay for parent) in general, so anything beyond that would be "extra". Just trying ancestor sets of consecutive linearization elements (whenever their joint feerate is high enough) sounds sufficient to me, but I'm still wondering about:
* whether something simpler is possible that's equally good.
* whether there is a more general way to t
...
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1622227095)
@ajtowns Yes, I don't think we have any requirement or even strong desire to go beyond ancestor-set-based linearization quality (= sufficient for single child pay for parent) in general, so anything beyond that would be "extra". Just trying ancestor sets of consecutive linearization elements (whenever their joint feerate is high enough) sounds sufficient to me, but I'm still wondering about:
* whether something simpler is possible that's equally good.
* whether there is a more general way to t
...
💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253455657)
update: no longer needed
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253455657)
update: no longer needed
💬 ishaanam commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253447995)
In 232edb7e5d630d9d687ba3089aa4028f8e0380a4 " Bump unconfirmed parent txs to target feerate "
Not directly related to this PR, but it would be helpful to write a comment about how the second `COutput` constructor (beneath this one) is only used in tests.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253447995)
In 232edb7e5d630d9d687ba3089aa4028f8e0380a4 " Bump unconfirmed parent txs to target feerate "
Not directly related to this PR, but it would be helpful to write a comment about how the second `COutput` constructor (beneath this one) is only used in tests.
💬 ishaanam commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1235758654)
In 232edb7e5d630d9d687ba3089aa4028f8e0380a4 " Bump unconfirmed parent txs to target feerate "
Why was this added?
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1235758654)
In 232edb7e5d630d9d687ba3089aa4028f8e0380a4 " Bump unconfirmed parent txs to target feerate "
Why was this added?
💬 ishaanam commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1235753039)
It would be good to have a test for the application of ancestor bump fees with external pre-selected inputs.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1235753039)
It would be good to have a test for the application of ancestor bump fees with external pre-selected inputs.
💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253478816)
https://cirrus-ci.com/task/5531648075235328?logs=ci#L4421
Actually there is still a race condition there. I think the keep_alive is necessary for bitcoind to even acknowledge the blocks only peer exists. We can use debug log or getpeerinfo but seems like either way the socks proxy is closing too soon
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253478816)
https://cirrus-ci.com/task/5531648075235328?logs=ci#L4421
Actually there is still a race condition there. I think the keep_alive is necessary for bitcoind to even acknowledge the blocks only peer exists. We can use debug log or getpeerinfo but seems like either way the socks proxy is closing too soon
💬 pinheadmz commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1253502850)
Yep you are absoutley right, I'll remove.
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1253502850)
Yep you are absoutley right, I'll remove.
💬 pinheadmz commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1253521412)
I added a log message but not sure what else to do. The problem is that `getaddrinfo()` is blocking and `getaddrinfo_a()` segfaults 😭 As long as the DNS request resolves *eventually* the thread will close, even if bitcoind has given up and detached the thread. If `getaddrinfo()` hangs forever then yes you're right that is a leak, but the user also has a serious networking issue.
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1253521412)
I added a log message but not sure what else to do. The problem is that `getaddrinfo()` is blocking and `getaddrinfo_a()` segfaults 😭 As long as the DNS request resolves *eventually* the thread will close, even if bitcoind has given up and detached the thread. If `getaddrinfo()` hangs forever then yes you're right that is a leak, but the user also has a serious networking issue.
💬 Brotcrunsher commented on pull request "DRAFT: Checking for multi/single-value types in UniValue.":
(https://github.com/bitcoin/bitcoin/pull/27994#issuecomment-1622330328)
Closing. Although this might be valid, I currently lack the time to check this in detail. If I find the time to properly check it all, I might reopen it.
(https://github.com/bitcoin/bitcoin/pull/27994#issuecomment-1622330328)
Closing. Although this might be valid, I currently lack the time to check this in detail. If I find the time to properly check it all, I might reopen it.